Raise error-message compliance for sandbox and workflow run validation paths - #52176
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. 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. 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
- "api.individual.githubcopilot.com"See Network Configuration for more information.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the implementation label and has 38 new lines of code in business logic directories (threshold: 100).
|
|
✅ Test Quality Sentinel completed test quality analysis.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship. PR only rewords error message strings (no new abstractions, dependencies, or control flow); nothing in scope for over-engineering review.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 28.9 AIC · ⌖ 6.2 AIC · ⊞ 5.4K
Comments that could not be inline-anchored
pkg/workflow/sandbox_validation.go:260
The new message drops the actionable hint to also enable sandbox.agent.legacy-security. The original message told the user to "declare it under services: with a port mapping and enable sandbox.agent.legacy-security". The replacement example shows only a services: block, so users who add the services: block but forget legacy-security will get no further guidance.
Suggested fix – append the missing hint:
return fmt.Errorf("allow-host-ports value %d maps to blocked service…
</details>There was a problem hiding this comment.
Pull request overview
Standardizes CLI and sandbox validation errors with actionable expectations and recovery examples, updating pinned test assertions accordingly.
Changes:
- Improves sandbox, workflow-run, project, and add-command error messages.
- Aligns related test assertions with revised wording.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/sandbox_validation.go |
Revises sandbox validation errors. |
pkg/workflow/sandbox_validation_test.go |
Updates sandbox error assertions. |
pkg/cli/run_workflow_execution.go |
Revises workflow-run errors. |
pkg/cli/run_workflow_execution_test.go |
Updates workflow-run assertions. |
pkg/cli/project_command.go |
Improves project validation errors. |
pkg/cli/add_command.go |
Improves add-command errors. |
pkg/cli/add_command_test.go |
Updates add-command assertion. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Suppressed comments (2)
pkg/cli/run_workflow_execution.go:548
- This points recovery at the generated lock file, although the trigger should be added to the source workflow frontmatter and then compiled. Naming the source and recompilation step avoids directing users toward an edit that will be overwritten.
return fmt.Errorf("workflow '%s' does not declare a workflow_dispatch trigger, so it cannot be run manually on GitHub Actions. Expected a workflow_dispatch trigger in the lock file. Example: on: workflow_dispatch", workflowName)
pkg/cli/run_workflow_execution.go:621
- The example passes a generated
.lock.ymlfile, but the run path resolves a source workflow andIsRunnableappends.lock.ymlitself (run_workflow_validation.go:25-36). Following this example makes it look forci.lock.yml.lock.yml; use the workflow name or.mdsource instead.
return errors.New("workflow list is empty. Expected at least one workflow file name or numeric workflow ID. Example: gh aw run ci.lock.yml")
- Files reviewed: 7/7 changed files
- Comments generated: 5
- Review effort level: Balanced
| for _, port := range ports { | ||
| if port < minPort || port > maxPort { | ||
| return fmt.Errorf("invalid allow-host-ports value: %d. Expected a TCP port between 1 and 65535. Example: allow-host-ports: [5432]", port) | ||
| return fmt.Errorf("allow-host-ports value %d is out of range. Expected a TCP port between 1 and 65535. Example: allow-host-ports: [5432]", port) |
| } | ||
| if service, dangerous := awfDangerousHostPorts[port]; dangerous { | ||
| return fmt.Errorf("invalid allow-host-ports value: %d. This port is blocked by AWF as a dangerous port (%s) and cannot be reached via allow-host-ports even in legacy-security mode. To reach a service on this port, declare it under services: with a port mapping and enable sandbox.agent.legacy-security", port, service) | ||
| return fmt.Errorf("allow-host-ports value %d maps to blocked service %s. Expected blocked service ports to be removed from allow-host-ports because they remain unreachable there, even with legacy-security. Example:\n# Do not list blocked service ports under allow-host-ports\nservices:\n db:\n image: postgres\n ports: [\"5432:5432\"]", port, service) |
| } | ||
| if !runnable { | ||
| return fmt.Errorf("workflow '%s' cannot be run on GitHub Actions - it must have 'workflow_dispatch' trigger", workflowIdOrName) | ||
| return fmt.Errorf("workflow '%s' does not declare a workflow_dispatch trigger, so it cannot be run manually on GitHub Actions. Expected a workflow_dispatch trigger in the lock file. Example: on: workflow_dispatch", workflowIdOrName) |
|
|
||
| if scope == "" { | ||
| return projectURLInfo{}, errors.New("invalid project URL: could not find orgs/users segment") | ||
| return projectURLInfo{}, errors.New("project URL is missing an 'orgs' or 'users' segment. Expected https://github.com/orgs/<org>/projects/<number> or https://github.com/users/<user>/projects/<number>. Example: https://github.com/users/octocat/projects/123") |
Test Quality Sentinel Report 🧪PR: #52176 — "Raise error-message compliance for sandbox and workflow run validation paths" Analysis SummaryKey Finding: No New Test FunctionsThis PR contains test assertion updates only:
The changes update error message assertions to match new error message copy in the production code:
Classification: Assertion MaintenanceThese are test maintenance changes, not new behavioral tests. The existing tests remain valid:
Test Quality ScoreScore: N/A — Test Maintenance Only This PR contains no new test functions and no test infrastructure changes. It is purely assertion maintenance to keep test expectations synchronized with updated error message text. No numeric scoring applies. Violations Check✅ No violations detected:
Recommendation✅ APPROVED — This is maintenance work that ensures tests accurately reflect the error messages they are validating. The existing tests continue to verify error conditions correctly.
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: Test maintenance only. This PR updates error message assertions to match new error copy in production code. All 8 error assertions verify non-happy-path conditions with descriptive messages. No new test functions, infrastructure, or mock usage. No violations detected.
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 32.7 AIC · ⌖ 6.68 AIC · ⊞ 7K
Comment /matt to run again
| // Check if GitHub CLI is available | ||
| if !isGHCLIAvailable() { | ||
| return nil, errors.New("GitHub CLI (gh) is required for PR creation but not available") | ||
| return nil, errors.New("GitHub CLI (gh) is not available. Expected gh to be installed and on PATH before using --create-pull-request. Example: brew install gh") |
There was a problem hiding this comment.
[/codebase-design] Platform-specific install hint — brew install gh is macOS-only and will mislead Windows/Linux users.
💡 Suggestion
Replace with a cross-platform reference:
return nil, errors.New("GitHub CLI (gh) is not available. Expected gh to be installed and on PATH before using --create-pull-request. See: https://cli.github.com/")The same pattern also appears in run_workflow_execution.go line 87.
@copilot please address this.
| return fmt.Errorf("allow-host-ports value %d maps to blocked service %s. Expected blocked service ports to be removed from allow-host-ports because they remain unreachable there, even with legacy-security. Example:\n# Do not list blocked service ports under allow-host-ports\nservices:\n db:\n image: postgres\n ports: [\"5432:5432\"]", port, service) | ||
| } | ||
| } | ||
| return nil |
There was a problem hiding this comment.
[/codebase-design] The new blocked-port error embeds a YAML comment (# Do not list blocked service ports under allow-host-ports) inside the error message string.
💡 Suggestion
YAML comments in error text are confusing outside a YAML context and add noise. Replace with a plain prose sentence:
return fmt.Errorf("allow-host-ports value %d maps to blocked service %s. That port is always unreachable from allow-host-ports, even with legacy-security. Declare the service under services: instead. Example:\nservices:\n db:\n image: postgres\n ports: [\"5432:5432\"]", port, service)@copilot please address this.
| func getSandboxDisableJustification(workflowData *WorkflowData) (string, error) { | ||
| if workflowData == nil || workflowData.Features == nil { | ||
| return "", errors.New("dangerously-disable-sandbox-agent feature is missing") | ||
| return "", errors.New("features block is missing dangerously-disable-sandbox-agent configuration. Expected a non-empty string justification under features when sandbox.agent is false. Example:\nfeatures:\n dangerously-disable-sandbox-agent: \"Temporary migration while hardening container profile\"") |
There was a problem hiding this comment.
[/tdd] Two error paths — workflowData == nil and workflowData.Features == nil — now emit the same message, making it impossible to distinguish them in tests or logs.
💡 Suggestion
Give each branch a distinct message (they already existed as separate branches before this PR):
if workflowData == nil {
return "", errors.New("workflow data is nil; cannot read dangerously-disable-sandbox-agent configuration")
}
if workflowData.Features == nil {
return "", errors.New("features block is missing dangerously-disable-sandbox-agent configuration. ...")
}This also makes test assertions more precise.
@copilot please address this.
| data, ok := response["data"].(map[string]any) | ||
| if !ok { | ||
| return nil, errors.New("invalid response: missing 'data' field") | ||
| return nil, errors.New("response is missing the 'data' field. Expected the GitHub GraphQL mutation payload to include data.createProjectV2.projectV2. Example: run 'gh auth status' to verify token scopes, then retry the command") |
There was a problem hiding this comment.
[/codebase-design] Three consecutive GraphQL response-field errors share the same message and recovery hint, making them indistinguishable in logs.
💡 Suggestion
Each error already names the missing field. The gh auth status recovery hint is generic and unlikely to help for createProjectV2/projectV2 fields. Consider marking inner checks as internal errors:
return nil, errors.New("response is missing 'data' field (internal error)")
return nil, errors.New("response is missing 'createProjectV2' field (internal error)")
return nil, errors.New("response is missing 'projectV2' field (internal error)")This keeps the gh auth status hint only at the top-level where it is actionable.
@copilot please address this.
|
@copilot please address the latest review feedback, refresh the branch if GitHub offers it, and run the pr-finisher skill. Open review items (newest first):
Please push a follow-up commit and resolve any addressed review threads.
|
🤖 Triage: PR #52176
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in 5024851: restored the |
This PR addresses the low-compliance error-message findings from
lint-error-messagesfor the targeted batch by standardizing messages to the repo style: what failed, what is expected, and a concrete example/recovery hint. Changes are limited to message text (plus assertion updates where tests pinned old phrasing), with no control-flow or behavior changes.Targeted files and scope
pkg/workflow/sandbox_validation.gopkg/cli/run_workflow_execution.gopkg/cli/project_command.gopkg/cli/add_command.gopkg/workflow/memory_validation_config.gois not present in this checkout;pkg/workflow/repo_memory_validation.gois already compliant and unchanged.Validation-message rewrite patterns applied
%wwrapping and existing error boundaries for operational paths.Notable message classes improved
--owner, workflow args,--input key=value).workflow_dispatchrequirement).Test alignment (message-fragment assertions only)
pkg/cli/add_command_test.gopkg/cli/run_workflow_execution_test.gopkg/workflow/sandbox_validation_test.goExample of the new error style: