Fix visibility for organization and enterprise variable creation#56267
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
❌ Design Decision Gate 🏗️ failed during design decision gate check. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
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.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
Non-blocking from a code-quality standpoint.
Why I'm not blocking this
This change fixes the missing visibility on org/enterprise variable creation, keeps PATCH behavior unchanged so existing visibility is preserved, and adds focused tests around both argument construction and error handling. I did not find a correctness or maintainability issue in the changed lines that is strong enough to block merge.🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 7.49 AIC · ⌖ 6.86 AIC · ⊞ 7K
Comment /review to run again
There was a problem hiding this comment.
Review: Fix visibility for organization and enterprise variable creation
This is a clean, well-scoped bug fix. The changes correctly thread a --visibility flag through validation → target resolution → API call for org/ent scopes, and the test coverage is thorough (argument presence, PATCH-first semantics, error branches, flag validation).
One non-blocking concern (harden): The visibility-specific error hint in upsertDefaultsVariable uses a fragile string search on the raw API response (strings.Contains(strings.ToLower(string(out)), "visibility")). If the API error message ever changes wording, or if an unrelated field named "visibility" appears in a different error, the hint could silently disappear or appear spuriously. Since the visibility parameter is always mandatory for org/ent creates, surfacing the hint unconditionally for all create failures at those scopes would be simpler and more robust — see the inline comment for details.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 26.8 AIC · ⌖ 9.21 AIC · ⊞ 6.2K
| if (target.scope == defaultsScopeOrg || target.scope == defaultsScopeEnt) && | ||
| strings.Contains(strings.ToLower(string(out)), "visibility") { | ||
| scopeName := "organization" | ||
| example := "gh aw env update defaults.yml --scope org --org my-org --visibility all" |
There was a problem hiding this comment.
The visibility-specific error branch relies on a string search for "visibility" in the raw API response body:
strings.Contains(strings.ToLower(string(out)), "visibility")This is a brittle heuristic — any API error that incidentally mentions "visibility" (e.g. an unrelated 422 about a different field, or a future error message that changes wording) would trigger the misleading "variables require a visibility" hint even when the real failure is a permissions error, bad slug, or quota issue.
Consider always including the visibility hint for all create failures at org/ent scope, since it is always valid context:
if target.scope == defaultsScopeOrg || target.scope == defaultsScopeEnt {
// always surface the visibility hint for create failures
}This makes the message both more reliable and more helpful without risking false negatives if the API response wording changes. @copilot please address this.
There was a problem hiding this comment.
Kept the guidance limited to the actual HTTP 422 missing-visibility response so policy and permission failures retain their original error context. The classifier and tests now cover this distinction in pkg/cli/env_command.go.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — requesting changes on two issues in upsertDefaultsVariable.
📋 Key Themes & Highlights
Key Themes
- Fragile post-fix heuristic: The
strings.Contains(..., "visibility")error branch (line 580) is likely dead code now that visibility is always sent, and it can false-positive on unrelated API errors. - Error construction style:
console.FormatErrorMessageis called at error construction time (line 588) rather than at display time, breaking the error-chain pattern used everywhere else in this file.
Positive Highlights
- ✅ Root cause properly fixed — visibility is now sent unconditionally on POST for org/ent scope
- ✅ Excellent test coverage: new tests cover PATCH→POST fallback, existing-variable preservation, visibility error, and unrelated error paths
- ✅
validateDefaultsVisibilityis cleanly separated and independently tested - ✅ PATCH path correctly left unchanged — existing variables preserve their visibility
- ✅ Documentation and changelog updated consistently
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 30.5 AIC · ⌖ 10.1 AIC · ⊞ 7.6K
Comment /matt to run again
| if err != nil { | ||
| return fmt.Errorf("failed to set %s: %w", name, errWithOutput(err, out)) | ||
| if (target.scope == defaultsScopeOrg || target.scope == defaultsScopeEnt) && | ||
| strings.Contains(strings.ToLower(string(out)), "visibility") { |
There was a problem hiding this comment.
[/diagnosing-bugs] The visibility-error heuristic is fragile and likely dead code after this fix. Because visibility is now unconditionally sent in the POST body (line 575), the API should never respond with a "visibility missing" error. If it does fire, any API error whose response body happens to mention "visibility" for an unrelated reason (e.g. "invalid visibility value") will show the misleading "variables require a visibility" hint even though the user already supplied one.
💡 Suggested approach
Since visibility is always sent now, remove the strings.Contains(..., "visibility") special-case entirely and simplify to:
return fmt.Errorf("failed to create %s at %s scope: %w", name, target.scope, errWithOutput(err, out))Add a test case where the POST fails with a non-visibility error (e.g. HTTP 403) to confirm the plain error message is surfaced without noise.
@copilot please address this.
There was a problem hiding this comment.
The special case now applies only to HTTP 422 missing-visibility responses; unrelated create failures use the plain error wrapper. Added coverage for an invalid visibility response in 91b6e04.
| example = "gh aw env update defaults.yml --scope ent --enterprise my-ent --visibility all" | ||
| } | ||
| message := fmt.Sprintf("failed to create %s at %s scope: %s variables require a visibility. Expected one of all, private, selected. Example: %s", name, target.scope, scopeName, example) | ||
| return fmt.Errorf("%s: %w", console.FormatErrorMessage(message), errWithOutput(err, out)) |
There was a problem hiding this comment.
[/diagnosing-bugs] Double-wrapping console.FormatErrorMessage inside fmt.Errorf with %w leaks styled/formatted markup into the error chain. When this error is unwrapped or displayed in a non-terminal context (logs, test output), the caller sees both the styled prefix and the raw errWithOutput tail, which looks inconsistent. Compare with line 590, which formats cleanly without FormatErrorMessage.
💡 Suggested fix
Either format only the message without %w, or (better) separate the user-facing message from the wrapped error, consistent with the pattern on line 590:
// cleaner — consistent with line 590
return fmt.Errorf("%s: %w", message, errWithOutput(err, out))
// where `message` is the plain string (no FormatErrorMessage wrapping)If styled output is required here, apply FormatErrorMessage only at the point the error is displayed to the user, not when it is constructed.
@copilot please address this.
There was a problem hiding this comment.
Removed console.FormatErrorMessage from the wrapped error in 91b6e04, leaving a plain message and the underlying command error.
Test Quality Sentinel 🧪PR: #56267 — Fix visibility for organization and enterprise variable creation Summary✅ Score: 80/100 (Excellent) 1 new test added, 1 design-contract test, 1 edge-case scenario covered. Test verifies the critical fix: unrelated errors (e.g., HTTP 403) no longer trigger visibility-requirement messaging. Test Metrics
Test AnalysisTestUpsertDefaultsVariableCreateUnrelatedError — Design test, high valuePurpose: Validates error messaging when variable creation fails with an unrelated error (HTTP 403 Forbidden). The fix ensures visibility-related guidance only appears when visibility is actually the issue. Assertions:
Design contract: When an org/enterprise variable creation fails with a non-visibility error, the error message MUST NOT include visibility guidance. Regression value: HIGH — Without this test, a future change could accidentally re-introduce the false visibility suggestion for all errors. FlagsWhile the test quality is high (comprehensive edge-case coverage), the length-to-production-change ratio exceeds the 2:1 threshold. This is not wasteful — the test is properly structured and validates a critical error path — but noted for visibility. Approval✅ No hard violations (build tags present, no mock library usage, all assertions descriptive). Test covers behavioral contract with appropriate edge-case scenario.
|
There was a problem hiding this comment.
Pull request overview
Fixes organization and enterprise variable creation by supplying visibility while preserving existing visibility during updates.
Changes:
- Adds and validates
--visibility. - Sends visibility only on organization/enterprise POST requests.
- Adds tests and documentation.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/env_command.go |
Implements visibility handling and API errors. |
pkg/cli/env_command_test.go |
Tests visibility validation and requests. |
docs/src/content/docs/reference/governance.md |
Documents visibility behavior. |
docs/src/content/docs/reference/compiler-enterprise-environment-controls.md |
Adds usage examples. |
CHANGELOG.md |
Records the fix. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
| if tt.expectedVisibility == "" { | ||
| assert.NotContains(t, calls[1], "visibility=") | ||
| } else { | ||
| assert.Contains(t, calls[1], tt.expectedVisibility) | ||
| } |
There was a problem hiding this comment.
Updated the repository-create assertion to reject any argument with the visibility= prefix in 91b6e04.
| if (target.scope == defaultsScopeOrg || target.scope == defaultsScopeEnt) && | ||
| strings.Contains(strings.ToLower(string(out)), "visibility") { |
There was a problem hiding this comment.
Restricted the hint to HTTP 422 missing-field signatures and added a test showing an invalid visibility response retains the generic error in 91b6e04.
|
Category: bug | Risk: high | Score: 70 (impact 36 + urgency 22 + quality 12) | Action: fast_track | Batch: permissions-security | Note: Variable creation fix with enterprise/org behavior change.
|
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
gh aw env updatecould update existing organization and enterprise variables but failed to create them because POST requests omitted the requiredvisibilityfield.CLI behavior
--visibility all|private|selected, defaulting toall.Variable creation
Documentation
gh aw env update --scope orgcannot create variables that do not already exist (HTTP 422: missingvisibility) #56088