[WIP] Fix failing GitHub Actions job Integration: Workflow Misc Part 2#38265
Merged
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
… feature flag Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
|
❌ Test Quality Sentinel was skipped during test quality analysis. |
Contributor
|
🧠 Matt Pocock Skills Reviewer was skipped during the skills-based review. |
Contributor
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #38265 does not have the "implementation" label and has 75 new lines of code in business logic directories (below the 100-line threshold). |
Contributor
|
✅ PR Code Quality Reviewer completed the code quality review. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates gh-aw’s integration test expectations and a large set of generated workflow lock files to address failures caused by changes in how AWF config JSON is embedded/escaped in lock YAML, and to surface additional agent error signals via a centralized “Detect agent errors” step.
Changes:
- Update integration tests to include the required
features.dangerously-disable-sandbox-agentjustification whensandbox.agent: falseis used. - Make several integration tests more tolerant of shell/YAML escaping differences for embedded AWF config JSON (e.g.,
allowDomains,blockDomains, api-target JSON). - Regenerate many
.github/workflows/*.lock.ymlworkflows to add aDetect agent errorsstep and propagate new job outputs/env vars (inference/MCP policy/model/timeout error flags).
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/sandbox_agent_disabled_test.go | Adds required sandbox-disable justification feature to integration test workflows using sandbox.agent: false. |
| pkg/workflow/domains_protocol_integration_test.go | Adjusts lock-file assertions to be more tolerant of escaped JSON key formatting. |
| pkg/workflow/blocked_domains_integration_test.go | Adjusts lock-file assertions for allowDomains/blockDomains presence checks to be more tolerant of escaping. |
| pkg/workflow/aw_info_steps_test.go | Adds required sandbox-disable justification feature for the “firewall disabled” integration test case. |
| pkg/workflow/allowed_domains_sanitization_test.go | Improves robustness of parsing/verification for embedded AWF config JSON when YAML escaping varies. |
| .github/workflows/unbloat-docs.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/typist.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/test-create-pr-error-handling.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/step-name-alignment.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/static-analysis-report.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/spec-enforcer.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/smoke-codex.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/smoke-claude.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/smoke-call-workflow.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/smoke-agent-scoped-approved.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/smoke-agent-public-none.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/smoke-agent-public-approved.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/smoke-agent-all-none.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/smoke-agent-all-merged.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/sergo.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/semantic-function-refactor.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/scout.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/schema-feature-coverage.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/schema-consistency-checker.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/safe-output-health.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/ruflo-backed-task.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/prompt-clustering-analysis.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/necromancer.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/lockfile-stats.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/issue-arborist.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/instructions-janitor.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/hourly-ci-cleaner.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/grumpy-reviewer.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/go-pattern-detector.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/go-logger.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/go-fan.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/github-mcp-tools-report.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/github-mcp-structural-analysis.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/example-workflow-analyzer.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/duplicate-code-detector.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/developer-docs-consolidator.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/dev.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/design-decision-gate.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/deep-report.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/daily-token-consumption-report.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/daily-team-evolution-insights.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/daily-security-red-team.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/daily-safeoutputs-git-simulator.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/daily-safe-outputs-conformance.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/daily-safe-output-optimizer.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/daily-rendering-scripts-verifier.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/daily-reliability-review.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/daily-observability-report.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/daily-multi-device-docs-tester.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/daily-function-namer.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/daily-fact.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/daily-doc-updater.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/daily-doc-healer.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/daily-code-metrics.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/daily-choice-test.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/daily-caveman-optimizer.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/daily-cache-strategy-analyzer.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/daily-awf-spec-compiler-surfacing.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/daily-aw-cross-repo-compile-check.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/daily-astrostylelite-markdown-spellcheck.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/daily-agentrx-trace-optimizer.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/copilot-session-insights.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/copilot-agent-analysis.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/commit-changes-analyzer.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/codex-github-remote-mcp-test.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/cloclo.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/cli-version-checker.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/claude-code-user-docs-review.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/ci-doctor.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/changeset.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/blog-auditor.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/aw-failure-investigator.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/avenger.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/audit-workflows.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/approach-validator.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/api-consumption-report.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
| .github/workflows/ai-moderator.lock.yml | Adds “Detect agent errors” step; propagates new agent error outputs/env vars. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 82/82 changed files
- Comments generated: 10
Comment on lines
+140
to
+143
| // The key appears shell-escaped in the lock file (\"allowDomains\" or "allowDomains"), | ||
| // so we search for the key name without surrounding quotes to match both forms. | ||
| if tt.checkAWFArgs { | ||
| if !strings.Contains(lockYAML, `"allowDomains"`) { | ||
| if !strings.Contains(lockYAML, "allowDomains") { |
Comment on lines
+307
to
+309
| // The key appears shell-escaped in the lock file (\"allowDomains\" or "allowDomains"), | ||
| // so we search for the key name without surrounding quotes to match both forms. | ||
| if !strings.Contains(lockYAML, "allowDomains") { |
Comment on lines
+69
to
+71
| // The key appears shell-escaped in the lock file (\"blockDomains\" or "blockDomains"), | ||
| // so we search for the key name without surrounding quotes to match both forms. | ||
| if !strings.Contains(lockYAML, "blockDomains") { |
Comment on lines
+85
to
+87
| // The key appears shell-escaped in the lock file (\"allowDomains\" or "allowDomains"), | ||
| // so we search for the key name without surrounding quotes to match both forms. | ||
| if !strings.Contains(lockYAML, "allowDomains") { |
Comment on lines
+148
to
+150
| // The key appears shell-escaped in the lock file (\"blockDomains\" or "blockDomains"), | ||
| // so we search for the key name without surrounding quotes to match both forms. | ||
| if !strings.Contains(lockYAML, "blockDomains") { |
Comment on lines
+218
to
+220
| // The key appears shell-escaped in the lock file (\"blockDomains\" or "blockDomains"), | ||
| // so we search for the key name without surrounding quotes to match both forms. | ||
| if strings.Contains(lockYAML, "blockDomains") { |
Comment on lines
+225
to
+227
| // The key appears shell-escaped in the lock file (\"allowDomains\" or "allowDomains"), | ||
| // so we search for the key name without surrounding quotes to match both forms. | ||
| if !strings.Contains(lockYAML, "allowDomains") { |
Comment on lines
+283
to
+285
| // The key appears shell-escaped in the lock file (\"blockDomains\" or "blockDomains"), | ||
| // so we search for the key name without surrounding quotes to match both forms. | ||
| if !strings.Contains(lockYAML, "blockDomains") { |
Comment on lines
+346
to
+348
| // The key appears shell-escaped in the lock file (\"blockDomains\" or "blockDomains"), | ||
| // so we search for the key name without surrounding quotes to match both forms. | ||
| if !strings.Contains(lockYAML, "blockDomains") { |
Comment on lines
+821
to
+824
| apiTargetCount := strings.Count(lockStr, apiTargetUnescaped) | ||
| if apiTargetCount == 0 { | ||
| apiTargetCount = strings.Count(lockStr, apiTargetEscaped) | ||
| } |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
REQUEST_CHANGES — 2 blocking issues, 4 additional concerns
The lock-file recompile (77 files, new detect_agent_errors step + 4 new job outputs) is mechanically correct. The test fixes for shell-escaped JSON keys are on the right track but the implementation has several correctness and reliability problems.
Blocking issues
1. Missing || 'false' in lock-file env vars (ai-moderator.lock.yml L1299 and all 77 recompiled files)
The four new GH_AW_* env vars lack the || 'false' fallback used by the existing GH_AW_AI_CREDITS_RATE_LIMIT_ERROR line directly above them. When the agent job is cancelled or skipped, needs.agent.outputs.* returns empty string (the job-level || 'false' never fires). Downstream != "false" guards would incorrectly trigger. Needs a source-template fix + make recompile.
2. Bare-string blockDomains in negative assertion (blocked_domains_integration_test.go L220)
Changing from "blockDomains" (quoted) to blockDomains (unquoted) is too broad for an absence guard. A future YAML comment or step-name mentioning blockDomains would cause a spurious test failure with no code change. Same loose pattern also weakens several positive-presence checks in the same file.
Non-blocking concerns (should fix before merging)
- Strict-mode test missing feature flag (
sandbox_agent_disabled_test.goL129–155): assumes strict-mode check fires before feature-flag check;assert.Containson the error message would silently become a false pass if that order ever changes. apiTargetCountOR-logic (allowed_domains_sanitization_test.goL821): exclusive-fallback pattern undercounts if escaped and unescaped occurrences mix; additive sum is strictly safer.allowDomainsPrefixloop commits to one form (allowed_domains_sanitization_test.goL837): occurrences in the non-selected form are silently skipped.- Silent
]-not-found fallback (allowed_domains_sanitization_test.goL622, L850): makesallowDomainsSectionthe entire rest of the file and produces unreliable results; should bet.Fatalf.
🔎 Code quality review by PR Code Quality Reviewer · ⌖ 14.4 AIC
Comments that could not be inline-anchored
pkg/workflow/blocked_domains_integration_test.go:220
"NOT present" assertion is too broad — high risk of spurious test failures. Using bare blockDomains (without quotes) matches anywhere in the YAML, including step names, comments, or log strings injected by the compiler, not just the JSON config key.
<details>
<summary>💡 Suggested fix</summary>
Check both the unescaped and escaped quoted-key forms, consistent with how allowed_domains_sanitization_test.go handles the same escaping problem:
// Check both unescaped and escaped-quo…
</details>
<details><summary>.github/workflows/ai-moderator.lock.yml:1299</summary>
**New error-flag env vars are missing `|| 'false'` fallback — will be empty string when the `agent` job is skipped or cancelled.** The immediately preceding `GH_AW_AI_CREDITS_RATE_LIMIT_ERROR` uses `|| 'false'` at the env-var level for exactly this reason; the four new vars omit it.
<details>
<summary>💡 Suggested fix</summary>
Consistent with the established pattern:
```yaml
GH_AW_INFERENCE_ACCESS_ERROR: ${{ needs.agent.outputs.inference_access_error || 'false' }}
GH_AW_MCP_POLICY_ERROR: ${…
</details>
<details><summary>pkg/workflow/sandbox_agent_disabled_test.go:153</summary>
**Strict-mode refusal test is missing the `dangerously-disable-sandbox-agent` feature flag — the `assert.Contains(t, err.Error(), "strict mode")` assertion relies on an undocumented check-ordering assumption.** Every other `sandbox.agent: false` fixture in this PR was updated to include the flag; this one was not.
<details>
<summary>💡 Why this matters</summary>
If the compiler validates the feature flag **before** strict mode, the error will say something like `"dangerously-disable-sandbox-a…
</details>
<details><summary>pkg/workflow/allowed_domains_sanitization_test.go:821</summary>
**`apiTargetCount` uses mutually-exclusive OR-logic that silently undercounts if both escaped and unescaped forms appear in the same file.** `if apiTargetCount == 0` gates the escaped-form count entirely; a file with 1 unescaped + 1 escaped occurrence would produce `apiTargetCount = 1`, failing the `< 2` check for the wrong reason.
<details>
<summary>💡 Suggested fix</summary>
Always sum both forms (they are mutually exclusive in practice, so summing adds no noise but removes the logic trap):…
</details>
<details><summary>pkg/workflow/allowed_domains_sanitization_test.go:837</summary>
**`allowDomainsPrefix` selection commits to one form for the entire loop, silently skipping any occurrences stored in the other form.** If the two AWF invocations (main agent + threat-detection) are ever serialised differently, this loop undercounts `occurrenceIdx` and silently skips domain-presence assertions for the missed occurrences.
<details>
<summary>💡 Suggested fix</summary>
Search for whichever prefix appears next in `remaining`, rather than pre-committing to one form at the start:
…
</details>
<details><summary>pkg/workflow/allowed_domains_sanitization_test.go:622</summary>
**Silent `]`-not-found fallback turns a malformed lock file into a silent false-pass, not an error.** If `strings.Index(lockStr[arrayStart:], "]")` returns -1, `allowDomainsEnd` is set to the full remaining length, making `allowDomainsSection` the entire rest of the file. Domain presence checks then run against all subsequent file content — expected domains produce false passes, unexpected domains produce arbitrary results.
<details>
<summary>💡 Suggested fix</summary>
```go
allowDomainsEnd :…
</details>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thanks for asking me to work on this. I will get started on it and keep this PR's description up to date as I form a plan and make progress.