-
Notifications
You must be signed in to change notification settings - Fork 494
Raise error-message compliance for sandbox and workflow run validation paths #52176
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
0514f46
16b34de
3bcde09
1df40b4
5024851
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -88,7 +88,7 @@ Project Setup: | |
| withProjectSetup, _ := cmd.Flags().GetBool("with-project-setup") | ||
|
|
||
| if owner == "" { | ||
| return errors.New("--owner flag is required. Use '@me' for current user or specify org name") | ||
| return errors.New("--owner flag is missing. Expected '@me' for the current user or an organization login. Example: gh aw project new \"My Project\" --owner @me") | ||
| } | ||
|
|
||
| config := ProjectConfig{ | ||
|
|
@@ -355,17 +355,17 @@ func createProject(ctx context.Context, ownerId, title string, verbose bool) (ma | |
| // Extract project data | ||
| 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") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/codebase-design] Three consecutive GraphQL response-field errors share the same message and recovery hint, making them indistinguishable in logs. 💡 SuggestionEach error already names the missing field. The 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 @copilot please address this. |
||
| } | ||
|
|
||
| createResult, ok := data["createProjectV2"].(map[string]any) | ||
| if !ok { | ||
| return nil, errors.New("invalid response: missing 'createProjectV2' field") | ||
| return nil, errors.New("response is missing the 'createProjectV2' 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") | ||
| } | ||
|
|
||
| project, ok := createResult["projectV2"].(map[string]any) | ||
| if !ok { | ||
| return nil, errors.New("invalid response: missing 'projectV2' field") | ||
| return nil, errors.New("response is missing the 'projectV2' 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") | ||
| } | ||
|
|
||
| console.LogVerbose(verbose, fmt.Sprintf("✓ Project created: #%v", project["number"])) | ||
|
|
@@ -380,7 +380,7 @@ func linkProjectToRepo(ctx context.Context, projectId, repoSlug string, verbose | |
| // Parse repo slug | ||
| parts := strings.Split(repoSlug, "/") | ||
| if len(parts) != 2 { | ||
| return fmt.Errorf("invalid repository format. Expected 'owner/repo', got '%s'", repoSlug) | ||
| return fmt.Errorf("repository slug '%s' is not in owner/repo format. Expected '<owner>/<repo>'. Example: github/gh-aw", repoSlug) | ||
| } | ||
| repoOwner := parts[0] | ||
| repoName := parts[1] | ||
|
|
@@ -451,7 +451,7 @@ func parseProjectURL(projectURL string) (projectURLInfo, error) { | |
| // Expected format: https://github.com/orgs/myorg/projects/123 or https://github.com/users/myuser/projects/123 | ||
| parts := strings.Split(projectURL, "/") | ||
| if len(parts) < 6 { | ||
| return projectURLInfo{}, errors.New("invalid project URL format") | ||
| return projectURLInfo{}, errors.New("project URL format is not recognized. Expected https://github.com/orgs/<org>/projects/<number> or https://github.com/users/<user>/projects/<number>. Example: https://github.com/orgs/github/projects/123") | ||
| } | ||
|
|
||
| var scope, ownerLogin, numberStr string | ||
|
|
@@ -467,7 +467,7 @@ func parseProjectURL(projectURL string) (projectURLInfo, error) { | |
| } | ||
|
|
||
| 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") | ||
|
|
||
| } | ||
|
|
||
| projectNumber, err := strconv.Atoi(numberStr) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -66,7 +66,7 @@ func validateMountsSyntax(mounts []string) error { | |
| fmt.Sprintf("Provide a valid destination path.\n\nExample:\nsandbox:\n mounts:\n - \"/host/path:/container/path:ro\"\n\nSee: %s", constants.DocsSandboxURL), | ||
| ) | ||
| default: | ||
| return fmt.Errorf("internal error: unsupported mount validation kind %d for sandbox mount %q", kind, mount) | ||
| return fmt.Errorf("internal error: sandbox mount validation kind %d for mount %q is not supported. Expected one of: invalid-format, too-few-parts, too-many-parts, empty-host-path, empty-destination. Example: \"/host/path:/container/path:ro\"", kind, mount) | ||
| } | ||
| }) | ||
| } | ||
|
|
@@ -532,38 +532,38 @@ func validateAgentMemoryLimit(memory string) error { | |
| func validateAllowHostPorts(ports []int) error { | ||
| 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: [9000]", 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 enabled; expose the service via GitHub Actions services: with sandbox.agent.legacy-security: enable instead. Example:\n# Do not list blocked service ports under allow-host-ports\nsandbox:\n agent:\n legacy-security: enable\nservices:\n db:\n image: postgres\n ports: [\"5432:5432\"]", port, service) | ||
| } | ||
| } | ||
| return nil | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/codebase-design] The new blocked-port error embeds a YAML comment ( 💡 SuggestionYAML 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\"") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] Two error paths — 💡 SuggestionGive 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. |
||
| } | ||
|
|
||
| flagName := string(constants.DangerouslyDisableSandboxAgentFeatureFlag) | ||
| value, found := getFeatureValueCaseInsensitive(workflowData.Features, flagName) | ||
| if !found { | ||
| return "", errors.New("dangerously-disable-sandbox-agent feature is missing") | ||
| return "", errors.New("dangerously-disable-sandbox-agent key is missing from features. 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\"") | ||
| } | ||
|
|
||
| justification, ok := value.(string) | ||
| if !ok { | ||
| return "", fmt.Errorf("feature must be a string, got %T", value) | ||
| return "", fmt.Errorf("dangerously-disable-sandbox-agent feature value has type %T. Expected a string justification. Example:\nfeatures:\n dangerously-disable-sandbox-agent: \"Temporary migration while hardening container profile\"", value) | ||
| } | ||
|
|
||
| trimmed := strings.TrimSpace(justification) | ||
| if len(trimmed) < minSandboxDisableJustificationLength { | ||
| return "", fmt.Errorf("feature must be at least %d characters", minSandboxDisableJustificationLength) | ||
| return "", fmt.Errorf("dangerously-disable-sandbox-agent justification is shorter than %d characters. Expected a descriptive justification string with at least %d characters. Example:\nfeatures:\n dangerously-disable-sandbox-agent: \"Temporary migration while hardening container profile\"", minSandboxDisableJustificationLength, minSandboxDisableJustificationLength) | ||
| } | ||
|
|
||
| if githubActionsExpressionPattern.MatchString(trimmed) { | ||
| return "", errors.New("feature cannot use GitHub Actions expressions") | ||
| return "", errors.New("dangerously-disable-sandbox-agent justification uses a GitHub Actions expression. Expected a literal explanatory string, not an expression. Example:\nfeatures:\n dangerously-disable-sandbox-agent: \"Temporary migration while hardening container profile\"") | ||
| } | ||
|
|
||
| return trimmed, nil | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/codebase-design] Platform-specific install hint —
brew install ghis macOS-only and will mislead Windows/Linux users.💡 Suggestion
Replace with a cross-platform reference:
The same pattern also appears in
run_workflow_execution.goline 87.@copilot please address this.