Enforce workspace-wide strict mode through aw.json - #59253
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 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
- "github.com"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. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories.
|
|
✅ Test Quality Sentinel completed test quality analysis. 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.
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Firewall blocked 4 domainsThe following domains were blocked by the firewall during workflow execution:
[!TIP] tools:
github:
mode: gh-proxySee GitHub Tools for more information on To allow these domains, add them to the network:
allowed:
- defaults
- "github.com/ghapi"
- "github.com"
- "pypi.org"
- "github.com/ghraw"See Network Configuration for more information.
|
|
✅ Ponytail Reviewer completed successfully! 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.
|
There was a problem hiding this comment.
Ponytail pass focused on deletable complexity in changed lines only.
net: -15 lines possible.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
ab.chatgpt.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
Generated by ✂️ Ponytail Reviewer for #59253 · codex · gpt53codex · 5.03 AIC · ⌖ 3.23 AIC · ⊞ 12.8K
Comment /ponytail to run again
| @@ -17,7 +18,8 @@ var compileOrchestratorLog = logger.New("cli:compile_orchestrator") | |||
| var compileUpdateContainerPins = updateContainerPins | |||
|
|
|||
| // CompileWorkflows compiles workflows based on the provided configuration | |||
There was a problem hiding this comment.
pkg/cli/compile_orchestrator.go:L20: yagni: workspace strict config loading embedded in CLI orchestrator. Reuse compiler-level strict resolution only; remove applyWorkspaceStrictMode from CLI.
There was a problem hiding this comment.
🟡 Changes recommended
Direct compiler entry points still execute several strict-sensitive paths using non-effective strict state.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds repository-wide strict compilation through aw.json, overriding workflow-level opt-outs.
Changes:
- Adds and validates the repository
strictsetting. - Applies repository precedence across compilation and security checks.
- Adds tests and documentation.
File summaries
| File | Description |
|---|---|
pkg/workflow/repo_config.go |
Parses repository strict mode. |
pkg/workflow/repo_config_test.go |
Tests accepted and rejected values. |
pkg/workflow/pull_request_target_validation.go |
Uses effective strict precedence. |
pkg/workflow/pull_request_target_validation_test.go |
Updates precedence expectations. |
pkg/workflow/compiler_yaml_policy.go |
Incorporates repository strict mode. |
pkg/workflow/compiler_repo_config_test.go |
Tests repository override behavior. |
pkg/parser/schemas/repo_config_schema.json |
Defines the strict setting schema. |
pkg/cli/compile_permissions_integration_test.go |
Tests CLI enforcement. |
pkg/cli/compile_orchestrator.go |
Applies workspace strict mode to CLI compilation. |
docs/src/content/docs/reference/frontmatter.md |
Documents workspace enforcement. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Balanced
| if repoConfig, err := c.loadRepoConfig(); err == nil && repoConfig.Strict { | ||
| compilerYAMLPolicyLog.Print("Strict mode enforced by repository config") | ||
| return true | ||
| } |
| // When the workflow frontmatter sets strict: false, effectiveStrictMode is lowered so the | ||
| // dangerous-trigger strict-only warning is skipped; the insecure-checkout check still runs | ||
| // and emits a non-strict warning when checkout is not explicitly disabled. |
🧪 Test Quality Sentinel ReportSummaryTest Quality Score: ✅ 85/100 — Excellent Verdict: ✅ APPROVE — Strong test coverage with design-focused assertions, proper error handling, and good edge-case coverage. Test Coverage OverviewFiles Analyzed
Total: 4 test files modified, 64 lines added, 3 lines deleted New & Modified Test Functions ReviewedClick to expand test classification table
Quality Findings✅ Strengths
Scoring BreakdownApproval Criteria Met✅ No Go mock library usage ( RecommendationApprove — The PR introduces well-structured tests for workspace-wide strict mode enforcement. Tests verify both the default behavior and the precedence rules (workspace config overrides workflow frontmatter). Error handling is properly validated, and code organization follows gh-aw conventions. 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.
|
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 4 domainsThe following domains were blocked by the firewall during workflow execution:
[!TIP] tools:
github:
mode: gh-proxySee GitHub Tools for more information on To allow these domains, add them to the network:
allowed:
- defaults
- "github.com/ghapi"
- "github.com"
- "pypi.org"
- "github.com/ghraw"See Network Configuration for more information.
|
There was a problem hiding this comment.
Verdict
Non-blocking from my pass: the repository-level strict precedence is wired through the compiler paths I checked, and I didn't find a changed-line regression that clearly warrants blocking this PR.
Reviewed themes
- Repository
aw.jsonstrict enforcement now wins over per-workflow frontmatter in both CLI orchestration and compiler-level validation paths. - The added schema/tests cover the intended
strict: trueacceptance andstrict: falserejection behavior. - I discarded the sub-agent output because it referenced unrelated files outside this PR.
Warning
Firewall blocked 4 domains
The following domains were blocked by the firewall during workflow execution:
github.com/ghapigithub.laiyagushi.compypi.orggithub.laiyagushi.com/ghraw
[!TIP]
github.com/ghapi is blocked because GitHub API access uses the built-in GitHub tools by default. Instead of adding github.com/ghapi to network.allowed, use tools.github.mode: gh-proxy for direct pre-authenticated GitHub CLI access without requiring network access to github.com/ghapi:
tools:
github:
mode: gh-proxySee GitHub Tools for more information on gh-proxy mode.
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "github.com/ghapi"
- "github.com"
- "pypi.org"
- "github.com/ghraw"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 60.3 AIC · ⌖ 7.34 AIC · ⊞ 23.5K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /grill-with-docs — requesting changes on a duplicated repo-config load path, unrelated lint suppressions, and a stale doc comment.
📋 Key Themes & Highlights
Key Themes
- Duplicated config loading:
applyWorkspaceStrictModeincompile_orchestrator.gore-implements git-root discovery +aw.jsonloading instead of reusing the compiler's cachedloadRepoConfig(), causingaw.jsonto be parsed twice and silently swallowing load errors that the rest of the compiler surfaces as warnings. - Unrelated lint suppressions: Two
(nolint/redacted):largefuncadditions (CompileWorkflows,validatePullRequestTargetTrigger) aren't explained by this PR's small diffs — one function actually got shorter. - Stale documentation: The
pull_request_target_validation.godoc comment still claims frontmatterstrict: falsealways disables the dangerous-trigger warning, which is no longer true when the repo enforces strict mode. - Test gap: precedence tests cover
aw.jsonstrict: true/false, but not the "aw.json omits strict" default case combined with frontmatterstrict: false.
Positive Highlights
- ✅ Schema correctly rejects
strict: falseat the repo-config level via"const": true. - ✅
effectiveStrictModecentralizes precedence cleanly andpull_request_target_validation.gonow reuses it instead of duplicating logic. - ✅ Both integration and unit tests were added for the new override behavior.
@copilot please address the review comments above.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 132.9 AIC · ⌖ 17.7 AIC · ⊞ 10.3K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/cli/compile_orchestrator.go:165
[/codebase-design] applyWorkspaceStrictMode re-derives gitRoot and calls workflow.LoadRepoConfig directly, duplicating (and bypassing the cache/warning behavior of) Compiler.loadRepoConfig(), which is used everywhere else (compiler_yaml_policy.go, configureGHESCompatibility). This means aw.json is now parsed twice per compile run, and a malformed aw.json here silently falls through (err == nil check discards the error) instead of surfacing the same warning the compiler emi…
pkg/cli/compile_orchestrator.go:21
[/codebase-design] The added (nolint/redacted):largefunc on CompileWorkflows is unrelated to this PR's change (a one-line call to applyWorkspaceStrictMode doesn't meaningfully grow the function). Suppressing a linter on an unrelated function inside a feature PR hides future large-function growth from review and should be justified/added separately if the function was already over the threshold before this change.
<details>
<summary>💡 Suggestion</summary>
Drop this nolint from this …
pkg/workflow/pull_request_target_validation.go:65
[/codebase-design] Same concern as in compile_orchestrator.go: this (nolint/redacted):largefunc addition isn't explained by the actual diff (which removes lines from this function, replacing 6 lines with 1 via c.effectiveStrictMode(...)). Adding a lint suppression to a function you just made shorter is confusing and should be dropped unless there's a broader repo-wide reason.
@copilot please address this.
pkg/workflow/pull_request_target_validation.go:14
[/grill-with-docs] The file-level doc comment still says "Workflows can opt out by setting strict: false in frontmatter," but this PR makes that statement false when repository-level aw.json sets strict: true (frontmatter opt-out is now overridden). This stale doc will mislead the next reader trying to understand precedence.
<details>
<summary>💡 Suggested fix</summary>
Update the comment to mention the new precedence, e.g.: "Workflows can opt out by setting strict: false in frontmatt…
pkg/workflow/repo_config_test.go:65
[/tdd] Good coverage for strict: true/strict: false at the LoadRepoConfig layer, but there's no test exercising the actual precedence behavior end-to-end for a workflow that sets strict: false in frontmatter while aw.json doesn't set strict at all (i.e. confirming the default repo-config path — no strict key present — still lets frontmatter strict: false through). Without this, a future regression that treats "key absent" the same as "strict: true" would go unnoticed.
@co…
@copilot Please take the next forward-progress pass on PR #59253.
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.
|
|
🎉 This pull request is included in a new release. Release: |
Allow repositories to enforce strict compilation across all workflows, preventing per-workflow
strict: falseopt-outs.Configuration
strictsupport to.github/workflows/aw.json.true; rejectfalseduring schema validation.Enforcement
Documentation
{ "strict": true }Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
github.laiyagushi.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.