Consolidate copilot-requests: write permission checks on Permissions

[Copilot]

The copilot-requests: write gate was implemented three times across workflow and CLI paths, with each call site re-deriving the same Permissions lookup. This change moves the check onto *Permissions so all callers share one source of truth for Copilot token-auth enablement.

• Shared permission helper

  • Add (*Permissions).HasCopilotRequestsWrite() to centralize the copilot-requests: write check, including safe nil handling.

• Call site consolidation

  • Route workflow execution permission checks through the shared helper.
  • Route compiler tip emission logic through the shared helper while preserving the existing explicit none override behavior.
  • Route CLI workflow secret detection through the shared helper after frontmatter parsing.

• Coverage

  • Add focused permission tests for shorthand, explicit, nil, and override cases.
  • Add validator-level coverage to verify the Copilot enablement tip still emits only when the write permission is not effectively granted.

func (p *Permissions) HasCopilotRequestsWrite() bool {
	if p == nil {
		return false
	}
	level, ok := p.Get(PermissionCopilotRequests)
	return ok && level == PermissionWrite
}

[github-actions]

✅ PR Code Quality Reviewer completed the code quality review.

[github-actions]

✅ Design Decision Gate 🏗️ completed the design decision gate check.

[github-actions]

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

[github-actions]

✅ Test Quality Sentinel completed test quality analysis.

[github-actions]

Clean, well-scoped refactor. The nil guard on HasCopilotRequestsWrite() is correct, the shouldEmitCopilotRequestsEnableTip semantics are preserved (existing explicit-none early return still fires before the helper), and test coverage addresses all meaningful cases. No blocking issues.

🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 9.63 AIC · ⌖ 5.37 AIC · ⊞ 4.8K

[github-actions]

Clean consolidation. Logic equivalence verified for all three call sites — the explicit PermissionNone early-return guard in shouldEmitCopilotRequestsEnableTip is preserved, nil safety is properly handled by the new method. Test coverage is thorough.

🔎 Code quality review by PR Code Quality Reviewer · 10 AIC · ⌖ 4.28 AIC · ⊞ 5.4K
_{Comment /review to run again}

[Copilot]

Pull request overview

This pull request consolidates the copilot-requests: write permission gate into a single (*Permissions).HasCopilotRequestsWrite() helper so workflow execution, compiler tip emission, and CLI secret detection all share the same permission-resolution logic (including nil-safe handling).

Changes:

• Added (*Permissions).HasCopilotRequestsWrite() and routed workflow-level checks through it.
• Updated compiler tip gating to use the shared helper while keeping the explicit copilot-requests: none override behavior.
• Updated CLI workflow secret detection to use the shared helper, and added focused unit tests covering shorthand/explicit/nil/override cases.

Show a summary per file

File	Description
pkg/workflow/permissions_operations.go	Adds HasCopilotRequestsWrite() and reuses it from the workflow permission gate.
pkg/workflow/permissions_operations_test.go	Adds unit tests for HasCopilotRequestsWrite() across shorthand, explicit, nil, and override scenarios.
pkg/workflow/permissions_compiler_validator.go	Switches Copilot enablement tip logic to use HasCopilotRequestsWrite() (retaining explicit none suppression).
pkg/workflow/compiler_validators_test.go	Adds validator-level tests to ensure the Copilot enablement tip emits/suppresses under the expected permission conditions.
pkg/cli/workflow_secrets.go	Routes CLI secret requirement filtering through HasCopilotRequestsWrite() after frontmatter parsing.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

• Files reviewed: 5/5 changed files
• Comments generated: 0
• Review effort level: Low

[github-actions]

Skills-Based Review 🧠

Applied /codebase-design — approving with one minor observation.

📋 Key Themes & Highlights

Key Themes

• Deep module: HasCopilotRequestsWrite() centralises the check correctly and handles nil safely — a good deepening move.
• Consistent pattern: follows the same shape as the existing HasContentsReadAccess().
• Semantic equivalence preserved: the shouldEmitCopilotRequestsEnableTip refactor keeps the explicit-none guard before delegating to the new helper, so the logic is unchanged.

Positive Highlights

• ✅ Nil-receiver guard in HasCopilotRequestsWrite() makes callers simpler.
• ✅ New tests cover nil, shorthand write-all, read-all, explicit write, explicit none, and the override case — solid specification coverage.
• ✅ The unexported hasCopilotRequestsWritePermission wrapper stays separate and correctly handles the CachedPermissions fallback concern.

Minor Observation

The new TestPermissions_HasCopilotRequestsWrite test uses bare t.Errorf while the sibling TestShouldEmitCopilotRequestsEnableTip (added in the same PR) uses assert.Equal. Not blocking, but aligning both to assert.Equal would match the project's dominant style.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 17.8 AIC · ⌖ 4.39 AIC · ⊞ 6.6K
_{Comment /matt to run again}

[github-actions]

🧪 Test Quality Sentinel Report

✅ Test Quality Score: 95/100 — Excellent

Analyzed 2 test(s): 2 design, 0 implementation, 0 violation(s).

📊 Metrics (2 tests)

Metric	Value
Analyzed	2 (Go: 2, JS: 0)
✅ Design	2 (100%)
⚠️ Implementation	0 (0%)
Edge/error coverage	2 (100%)
Duplicate clusters	0
Inflation	No
🚨 Violations	0

Test	File	Classification	Issues
TestShouldEmitCopilotRequestsEnableTip	compiler_validators_test.go	design_test / behavioral_contract / high_value	None
TestPermissions_HasCopilotRequestsWrite	permissions_operations_test.go	design_test / behavioral_contract / high_value	None

Both tests are table-driven and cover nil inputs, positive paths, and explicit-override edge cases (e.g., write-all global permission overridden by explicit none). Build tags (//go:build !integration) are present on both files. No mock library violations detected.

Verdict

✅ Passed. 0% implementation tests (threshold: 30%). Both tests enforce clear behavioral contracts on the new HasCopilotRequestsWrite() method and shouldEmitCopilotRequestsEnableTip() helper.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• awmgmcpg

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "awmgmcpg"

See Network Configuration for more information.

🧪 Test quality analysis by Test Quality Sentinel · 22.3 AIC · ⌖ 12.9 AIC · ⊞ 6.8K · ◷
_{Comment /review to run again}

[github-actions]

✅ Test Quality Sentinel: 95/100. 0% implementation tests (threshold: 30%).

[github-actions]

Design Decision Gate - ADR Required

This PR makes significant changes to core business logic (134 new lines in pkg/) but does not have a linked Architecture Decision Record (ADR).

Draft ADR committed: docs/adr/44650-consolidate-copilot-requests-write-permission.md - review and complete it before merging.

This PR cannot merge until an ADR is linked in the PR body.

What to do next

1. Review the draft ADR committed to your branch at docs/adr/44650-consolidate-copilot-requests-write-permission.md - it was generated from the PR diff
2. Complete any missing sections - add context the AI could not infer, refine the decision rationale, and confirm the alternatives listed are accurate
3. Commit the finalized ADR to docs/adr/ on your branch (or accept the draft as-is if it is accurate)
4. Reference the ADR in this PR body by adding a line such as: ADR: ADR-44650: Consolidate copilot-requests write permission

Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision.

Why ADRs Matter

ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you.

Michael Nygard ADR Format Reference

An ADR must contain these four sections to be considered complete:

• Context: What is the problem? What forces are at play?
• Decision: What did you decide? Why?
• Alternatives Considered: What else could have been done?
• Consequences: What are the trade-offs (positive and negative)?

All ADRs are stored in docs/adr/ as Markdown files numbered by PR number.

🏗️ ADR gate enforced by Design Decision Gate 🏗️ · 41.7 AIC · ⌖ 9.84 AIC · ⊞ 8.5K · ◷
_{Comment /review to run again}

[github-actions]

🎉 This pull request is included in a new release.

Release: v0.82.8