From f5e091623dec7e6402c9604856743bb0cf88bdaa Mon Sep 17 00:00:00 2001 From: "Claude Sonnet 4.5" Date: Fri, 13 Feb 2026 07:41:33 +0000 Subject: [PATCH 1/2] Add debug logging to 5 workflow-related files Enhanced debug logging across critical workflow compilation files for better troubleshooting. Added meaningful logging calls at key execution points. Files enhanced: - pkg/workflow/safe_outputs_config.go: Config extraction and handler setup - pkg/workflow/concurrency_validation.go: Expression validation progress - pkg/workflow/expression_builder.go: Condition tree construction - pkg/workflow/redact_secrets.go: Secret scanning and masking - pkg/workflow/markdown_security_scanner.go: Security category scans All logging follows guidelines: no side effects, meaningful messages, and proper use of log.Enabled() for expensive operations. Co-Authored-By: Claude Sonnet 4.5 --- pkg/workflow/concurrency_validation.go | 9 ++++++++ pkg/workflow/expression_builder.go | 8 +++++++ pkg/workflow/markdown_security_scanner.go | 26 ++++++++++++++++++++++- pkg/workflow/redact_secrets.go | 7 ++++++ pkg/workflow/safe_outputs_config.go | 13 ++++++++++++ 5 files changed, 62 insertions(+), 1 deletion(-) diff --git a/pkg/workflow/concurrency_validation.go b/pkg/workflow/concurrency_validation.go index 4fcf80b010d..a5a952d00e1 100644 --- a/pkg/workflow/concurrency_validation.go +++ b/pkg/workflow/concurrency_validation.go @@ -74,6 +74,7 @@ func validateConcurrencyGroupExpression(group string) error { // validateBalancedBraces checks that all ${{ }} braces are balanced and properly closed func validateBalancedBraces(group string) error { + concurrencyValidationLog.Print("Checking balanced braces in expression") openCount := 0 i := 0 positions := []int{} // Track positions of opening braces for error reporting @@ -111,6 +112,7 @@ func validateBalancedBraces(group string) error { if openCount > 0 { // Find the position of the first unclosed opening brace pos := positions[0] + concurrencyValidationLog.Printf("Found %d unclosed brace(s) starting at position %d", openCount, pos) return NewValidationError( "concurrency", "unclosed expression braces", @@ -119,6 +121,7 @@ func validateBalancedBraces(group string) error { ) } + concurrencyValidationLog.Print("Brace balance check passed") return nil } @@ -128,6 +131,10 @@ func validateExpressionSyntax(group string) error { expressionPattern := regexp.MustCompile(`\$\{\{([^}]*)\}\}`) matches := expressionPattern.FindAllStringSubmatch(group, -1) + if log.Enabled() { + concurrencyValidationLog.Printf("Found %d expression(s) to validate", len(matches)) + } + for _, match := range matches { if len(match) < 2 { continue @@ -189,7 +196,9 @@ func validateExpressionContent(expr string, fullGroup string) error { // Try to parse complex expressions with logical operators if containsLogicalOperators(expr) { + concurrencyValidationLog.Print("Expression contains logical operators, performing deep validation") if _, err := ParseExpression(expr); err != nil { + concurrencyValidationLog.Printf("Expression parsing failed: %v", err) return NewValidationError( "concurrency", "invalid expression syntax", diff --git a/pkg/workflow/expression_builder.go b/pkg/workflow/expression_builder.go index 6df005f3e7c..d0fc64d4399 100644 --- a/pkg/workflow/expression_builder.go +++ b/pkg/workflow/expression_builder.go @@ -58,6 +58,9 @@ func BuildOr(left ConditionNode, right ConditionNode) ConditionNode { // BuildAnd creates an AND node combining two conditions func BuildAnd(left ConditionNode, right ConditionNode) ConditionNode { + if log.Enabled() { + expressionBuilderLog.Print("Building AND condition node") + } return &AndNode{Left: left, Right: right} } @@ -81,6 +84,10 @@ func BuildReactionCondition() ConditionNode { } terms = append(terms, pullRequestCondition) + if log.Enabled() { + expressionBuilderLog.Printf("Created disjunction with %d event type terms", len(terms)) + } + // Use DisjunctionNode to avoid deep nesting return &DisjunctionNode{Terms: terms} } @@ -169,6 +176,7 @@ func BuildNotFromFork() *ComparisonNode { } func BuildSafeOutputType(outputType string) ConditionNode { + expressionBuilderLog.Printf("Building safe-output condition for output type: %s", outputType) // Use !cancelled() && needs.agent.result != 'skipped' to properly handle workflow cancellation // !cancelled() allows jobs to run when dependencies fail (for error reporting) // needs.agent.result != 'skipped' prevents running when workflow is cancelled (dependencies get skipped) diff --git a/pkg/workflow/markdown_security_scanner.go b/pkg/workflow/markdown_security_scanner.go index f1d7a495e65..f1865aec06e 100644 --- a/pkg/workflow/markdown_security_scanner.go +++ b/pkg/workflow/markdown_security_scanner.go @@ -68,6 +68,15 @@ func (f SecurityFinding) String() string { return fmt.Sprintf("[%s] %s", f.Category, f.Description) } +// countCategories counts unique security finding categories +func countCategories(findings []SecurityFinding) int { + categories := make(map[SecurityFindingCategory]bool) + for _, f := range findings { + categories[f.Category] = true + } + return len(categories) +} + // ScanMarkdownSecurity scans markdown content for dangerous or malicious patterns. // It automatically strips YAML frontmatter (delimited by ---) so that only the // markdown body is scanned. Line numbers in returned findings are adjusted to @@ -78,14 +87,23 @@ func ScanMarkdownSecurity(content string) []SecurityFinding { // Strip frontmatter and get the line offset for correct line number reporting markdownBody, lineOffset := stripFrontmatter(content) + if log.Enabled() { + markdownSecurityLog.Printf("Stripped frontmatter: %d line(s) removed, scanning %d bytes of markdown", lineOffset, len(markdownBody)) + } var findings []SecurityFinding + markdownSecurityLog.Print("Running unicode abuse detection") findings = append(findings, scanUnicodeAbuse(markdownBody)...) + markdownSecurityLog.Print("Running hidden content detection") findings = append(findings, scanHiddenContent(markdownBody)...) + markdownSecurityLog.Print("Running obfuscated links detection") findings = append(findings, scanObfuscatedLinks(markdownBody)...) + markdownSecurityLog.Print("Running HTML abuse detection") findings = append(findings, scanHTMLAbuse(markdownBody)...) + markdownSecurityLog.Print("Running embedded files detection") findings = append(findings, scanEmbeddedFiles(markdownBody)...) + markdownSecurityLog.Print("Running social engineering detection") findings = append(findings, scanSocialEngineering(markdownBody)...) // Adjust line numbers to account for stripped frontmatter @@ -98,7 +116,9 @@ func ScanMarkdownSecurity(content string) []SecurityFinding { } if len(findings) > 0 { - markdownSecurityLog.Printf("Found %d security issues in markdown content", len(findings)) + markdownSecurityLog.Printf("Security scan complete: found %d issue(s) across %d categor(ies)", len(findings), countCategories(findings)) + } else { + markdownSecurityLog.Print("Security scan complete: no issues found") } return findings @@ -177,6 +197,10 @@ func scanUnicodeAbuse(content string) []SecurityFinding { var findings []SecurityFinding lines := strings.Split(content, "\n") + if log.Enabled() { + markdownSecurityLog.Printf("Scanning %d line(s) for unicode abuse", len(lines)) + } + for lineNum, line := range lines { lineNo := lineNum + 1 diff --git a/pkg/workflow/redact_secrets.go b/pkg/workflow/redact_secrets.go index e98009735c1..5460ba64266 100644 --- a/pkg/workflow/redact_secrets.go +++ b/pkg/workflow/redact_secrets.go @@ -22,6 +22,7 @@ func escapeSingleQuote(s string) string { // CollectSecretReferences extracts all secret references from the workflow YAML // This scans for patterns like ${{ secrets.SECRET_NAME }} or secrets.SECRET_NAME func CollectSecretReferences(yamlContent string) []string { + secretMaskingLog.Printf("Scanning workflow YAML (%d bytes) for secret references", len(yamlContent)) secretsMap := make(map[string]bool) // Pattern to match ${{ secrets.SECRET_NAME }} or secrets.SECRET_NAME @@ -44,6 +45,10 @@ func CollectSecretReferences(yamlContent string) []string { // Sort for consistent output SortStrings(secrets) + if log.Enabled() { + secretMaskingLog.Printf("Found %d unique secret reference(s) in workflow", len(secrets)) + } + return secrets } @@ -59,11 +64,13 @@ func (c *Compiler) generateSecretRedactionStep(yaml *strings.Builder, yamlConten // If no secrets found, we still generate the step but it will be a no-op at runtime // This ensures consistent step ordering and validation if len(secretReferences) == 0 { + secretMaskingLog.Print("No secrets found, generating no-op redaction step") // Generate a minimal no-op redaction step for validation purposes yaml.WriteString(" - name: Redact secrets in logs\n") yaml.WriteString(" if: always()\n") yaml.WriteString(" run: echo 'No secrets to redact'\n") } else { + secretMaskingLog.Printf("Generating redaction step for %d secret(s)", len(secretReferences)) yaml.WriteString(" - name: Redact secrets in logs\n") yaml.WriteString(" if: always()\n") fmt.Fprintf(yaml, " uses: %s\n", GetActionPin("actions/github-script")) diff --git a/pkg/workflow/safe_outputs_config.go b/pkg/workflow/safe_outputs_config.go index d6c4b05f4ea..b4afae62c94 100644 --- a/pkg/workflow/safe_outputs_config.go +++ b/pkg/workflow/safe_outputs_config.go @@ -18,11 +18,15 @@ func (c *Compiler) extractSafeOutputsConfig(frontmatter map[string]any) *SafeOut if output, exists := frontmatter["safe-outputs"]; exists { if outputMap, ok := output.(map[string]any); ok { + if log.Enabled() { + safeOutputsConfigLog.Printf("Processing safe-outputs configuration with %d top-level keys", len(outputMap)) + } config = &SafeOutputsConfig{} // Handle create-issue issuesConfig := c.parseIssuesConfig(outputMap) if issuesConfig != nil { + safeOutputsConfigLog.Print("Configured create-issue output handler") config.CreateIssues = issuesConfig } @@ -89,6 +93,7 @@ func (c *Compiler) extractSafeOutputsConfig(frontmatter map[string]any) *SafeOut // Handle create-pull-request pullRequestsConfig := c.parsePullRequestsConfig(outputMap) if pullRequestsConfig != nil { + safeOutputsConfigLog.Print("Configured create-pull-request output handler") config.CreatePullRequests = pullRequestsConfig } @@ -126,6 +131,7 @@ func (c *Compiler) extractSafeOutputsConfig(frontmatter map[string]any) *SafeOut } } config.AllowedDomains = domainStrings + safeOutputsConfigLog.Printf("Configured allowed-domains with %d domain(s)", len(domainStrings)) } } @@ -399,11 +405,18 @@ func (c *Compiler) extractSafeOutputsConfig(frontmatter map[string]any) *SafeOut if outputMap, ok := output.(map[string]any); ok { if _, exists := outputMap["threat-detection"]; !exists { // Only apply default if threat-detection key doesn't exist + safeOutputsConfigLog.Print("Applying default threat-detection configuration") config.ThreatDetection = &ThreatDetectionConfig{} } } } } + if config != nil { + safeOutputsConfigLog.Print("Successfully extracted safe-outputs configuration") + } else { + safeOutputsConfigLog.Print("No safe-outputs configuration found in frontmatter") + } + return config } From c0587e99d8b8cc9d12e4dfa404091071d82f59a5 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Thu, 12 Feb 2026 23:54:18 -0800 Subject: [PATCH 2/2] [WIP] Address feedback on debug logging in workflow files (#15339) * Initial plan * Remove redundant if log.Enabled() checks in workflow files Removed unnecessary `if log.Enabled()` conditionals from 8 files in pkg/workflow/. The logger package already checks enabled state internally, making these extra conditionals redundant. None of the logging operations were expensive enough to warrant the conditional check (just counting lengths, time calculations). Files updated: - safe_outputs_config.go - redact_secrets.go - concurrency_validation.go - expression_builder.go (2 instances) - markdown_security_scanner.go (2 instances) - claude_tools.go - compiler.go Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .github/workflows/bot-detection.lock.yml | 8 ++++---- pkg/workflow/claude_tools.go | 4 +--- pkg/workflow/compiler.go | 4 +--- pkg/workflow/concurrency_validation.go | 4 +--- pkg/workflow/expression_builder.go | 8 ++------ pkg/workflow/markdown_security_scanner.go | 8 ++------ pkg/workflow/redact_secrets.go | 4 +--- pkg/workflow/safe_outputs_config.go | 4 +--- 8 files changed, 13 insertions(+), 31 deletions(-) diff --git a/.github/workflows/bot-detection.lock.yml b/.github/workflows/bot-detection.lock.yml index 1583e368877..644ae860f72 100644 --- a/.github/workflows/bot-detection.lock.yml +++ b/.github/workflows/bot-detection.lock.yml @@ -165,7 +165,7 @@ jobs: staged: false, allowed_domains: ["defaults"], firewall_enabled: true, - awf_version: "v0.16.2", + awf_version: "v0.16.3", awmg_version: "", steps: { firewall: "squid" @@ -189,7 +189,7 @@ jobs: - name: Install GitHub Copilot CLI run: /opt/gh-aw/actions/install_copilot_cli.sh 0.0.409 - name: Install awf binary - run: bash /opt/gh-aw/actions/install_awf_binary.sh v0.16.2 + run: bash /opt/gh-aw/actions/install_awf_binary.sh v0.16.3 - name: Determine automatic lockdown mode for GitHub MCP server id: determine-automatic-lockdown uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8 @@ -198,7 +198,7 @@ jobs: const determineAutomaticLockdown = require('/opt/gh-aw/actions/determine_automatic_lockdown.cjs'); await determineAutomaticLockdown(github, context, core); - name: Download container images - run: bash /opt/gh-aw/actions/download_docker_images.sh ghcr.io/github/gh-aw-firewall/agent:0.16.2 ghcr.io/github/gh-aw-firewall/squid:0.16.2 ghcr.io/github/gh-aw-mcpg:v0.1.4 ghcr.io/github/github-mcp-server:v0.30.3 node:lts-alpine + run: bash /opt/gh-aw/actions/download_docker_images.sh ghcr.io/github/gh-aw-firewall/agent:0.16.3 ghcr.io/github/gh-aw-firewall/squid:0.16.3 ghcr.io/github/gh-aw-mcpg:v0.1.4 ghcr.io/github/github-mcp-server:v0.30.3 node:lts-alpine - name: Write Safe Outputs Config run: | mkdir -p /opt/gh-aw/safeoutputs @@ -721,7 +721,7 @@ jobs: timeout-minutes: 10 run: | set -o pipefail - sudo -E awf --env-all --container-workdir "${GITHUB_WORKSPACE}" --allow-domains api.business.githubcopilot.com,api.enterprise.githubcopilot.com,api.github.com,api.githubcopilot.com,api.individual.githubcopilot.com,api.snapcraft.io,archive.ubuntu.com,azure.archive.ubuntu.com,crl.geotrust.com,crl.globalsign.com,crl.identrust.com,crl.sectigo.com,crl.thawte.com,crl.usertrust.com,crl.verisign.com,crl3.digicert.com,crl4.digicert.com,crls.ssl.com,github.com,host.docker.internal,json-schema.org,json.schemastore.org,keyserver.ubuntu.com,ocsp.digicert.com,ocsp.geotrust.com,ocsp.globalsign.com,ocsp.identrust.com,ocsp.sectigo.com,ocsp.ssl.com,ocsp.thawte.com,ocsp.usertrust.com,ocsp.verisign.com,packagecloud.io,packages.cloud.google.com,packages.microsoft.com,ppa.launchpad.net,raw.githubusercontent.com,registry.npmjs.org,s.symcb.com,s.symcd.com,security.ubuntu.com,telemetry.enterprise.githubcopilot.com,ts-crl.ws.symantec.com,ts-ocsp.ws.symantec.com --log-level info --proxy-logs-dir /tmp/gh-aw/sandbox/firewall/logs --enable-host-access --image-tag 0.16.2 --skip-pull \ + sudo -E awf --env-all --container-workdir "${GITHUB_WORKSPACE}" --allow-domains api.business.githubcopilot.com,api.enterprise.githubcopilot.com,api.github.com,api.githubcopilot.com,api.individual.githubcopilot.com,api.snapcraft.io,archive.ubuntu.com,azure.archive.ubuntu.com,crl.geotrust.com,crl.globalsign.com,crl.identrust.com,crl.sectigo.com,crl.thawte.com,crl.usertrust.com,crl.verisign.com,crl3.digicert.com,crl4.digicert.com,crls.ssl.com,github.com,host.docker.internal,json-schema.org,json.schemastore.org,keyserver.ubuntu.com,ocsp.digicert.com,ocsp.geotrust.com,ocsp.globalsign.com,ocsp.identrust.com,ocsp.sectigo.com,ocsp.ssl.com,ocsp.thawte.com,ocsp.usertrust.com,ocsp.verisign.com,packagecloud.io,packages.cloud.google.com,packages.microsoft.com,ppa.launchpad.net,raw.githubusercontent.com,registry.npmjs.org,s.symcb.com,s.symcd.com,security.ubuntu.com,telemetry.enterprise.githubcopilot.com,ts-crl.ws.symantec.com,ts-ocsp.ws.symantec.com --log-level info --proxy-logs-dir /tmp/gh-aw/sandbox/firewall/logs --enable-host-access --image-tag 0.16.3 --skip-pull \ -- '/usr/local/bin/copilot --add-dir /tmp/gh-aw/ --log-level all --log-dir /tmp/gh-aw/sandbox/agent/logs/ --add-dir "${GITHUB_WORKSPACE}" --disable-builtin-mcps --allow-all-tools --allow-all-paths --share /tmp/gh-aw/sandbox/agent/logs/conversation.md --prompt "$(cat /tmp/gh-aw/aw-prompts/prompt.txt)"${GH_AW_MODEL_AGENT_COPILOT:+ --model "$GH_AW_MODEL_AGENT_COPILOT"}' \ 2>&1 | tee /tmp/gh-aw/agent-stdio.log env: diff --git a/pkg/workflow/claude_tools.go b/pkg/workflow/claude_tools.go index b3768fe5d0a..46de4db36f6 100644 --- a/pkg/workflow/claude_tools.go +++ b/pkg/workflow/claude_tools.go @@ -389,9 +389,7 @@ func (e *ClaudeEngine) computeAllowedClaudeToolsString(tools map[string]any, saf // Sort the allowed tools alphabetically for consistent output sort.Strings(allowedTools) - if log.Enabled() { - claudeToolsLog.Printf("Generated allowed tools string with %d tools", len(allowedTools)) - } + claudeToolsLog.Printf("Generated allowed tools string with %d tools", len(allowedTools)) return strings.Join(allowedTools, ",") } diff --git a/pkg/workflow/compiler.go b/pkg/workflow/compiler.go index 060f2b92580..5eabf775b5a 100644 --- a/pkg/workflow/compiler.go +++ b/pkg/workflow/compiler.go @@ -517,9 +517,7 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath // Track compilation time for performance monitoring startTime := time.Now() defer func() { - if log.Enabled() { - log.Printf("Compilation completed in %v", time.Since(startTime)) - } + log.Printf("Compilation completed in %v", time.Since(startTime)) }() // Reset the step order tracker for this compilation diff --git a/pkg/workflow/concurrency_validation.go b/pkg/workflow/concurrency_validation.go index a5a952d00e1..5de2944581a 100644 --- a/pkg/workflow/concurrency_validation.go +++ b/pkg/workflow/concurrency_validation.go @@ -131,9 +131,7 @@ func validateExpressionSyntax(group string) error { expressionPattern := regexp.MustCompile(`\$\{\{([^}]*)\}\}`) matches := expressionPattern.FindAllStringSubmatch(group, -1) - if log.Enabled() { - concurrencyValidationLog.Printf("Found %d expression(s) to validate", len(matches)) - } + concurrencyValidationLog.Printf("Found %d expression(s) to validate", len(matches)) for _, match := range matches { if len(match) < 2 { diff --git a/pkg/workflow/expression_builder.go b/pkg/workflow/expression_builder.go index d0fc64d4399..1780a9791f8 100644 --- a/pkg/workflow/expression_builder.go +++ b/pkg/workflow/expression_builder.go @@ -58,9 +58,7 @@ func BuildOr(left ConditionNode, right ConditionNode) ConditionNode { // BuildAnd creates an AND node combining two conditions func BuildAnd(left ConditionNode, right ConditionNode) ConditionNode { - if log.Enabled() { - expressionBuilderLog.Print("Building AND condition node") - } + expressionBuilderLog.Print("Building AND condition node") return &AndNode{Left: left, Right: right} } @@ -84,9 +82,7 @@ func BuildReactionCondition() ConditionNode { } terms = append(terms, pullRequestCondition) - if log.Enabled() { - expressionBuilderLog.Printf("Created disjunction with %d event type terms", len(terms)) - } + expressionBuilderLog.Printf("Created disjunction with %d event type terms", len(terms)) // Use DisjunctionNode to avoid deep nesting return &DisjunctionNode{Terms: terms} diff --git a/pkg/workflow/markdown_security_scanner.go b/pkg/workflow/markdown_security_scanner.go index f1865aec06e..97b9d4b2907 100644 --- a/pkg/workflow/markdown_security_scanner.go +++ b/pkg/workflow/markdown_security_scanner.go @@ -87,9 +87,7 @@ func ScanMarkdownSecurity(content string) []SecurityFinding { // Strip frontmatter and get the line offset for correct line number reporting markdownBody, lineOffset := stripFrontmatter(content) - if log.Enabled() { - markdownSecurityLog.Printf("Stripped frontmatter: %d line(s) removed, scanning %d bytes of markdown", lineOffset, len(markdownBody)) - } + markdownSecurityLog.Printf("Stripped frontmatter: %d line(s) removed, scanning %d bytes of markdown", lineOffset, len(markdownBody)) var findings []SecurityFinding @@ -197,9 +195,7 @@ func scanUnicodeAbuse(content string) []SecurityFinding { var findings []SecurityFinding lines := strings.Split(content, "\n") - if log.Enabled() { - markdownSecurityLog.Printf("Scanning %d line(s) for unicode abuse", len(lines)) - } + markdownSecurityLog.Printf("Scanning %d line(s) for unicode abuse", len(lines)) for lineNum, line := range lines { lineNo := lineNum + 1 diff --git a/pkg/workflow/redact_secrets.go b/pkg/workflow/redact_secrets.go index 5460ba64266..0406ecf616d 100644 --- a/pkg/workflow/redact_secrets.go +++ b/pkg/workflow/redact_secrets.go @@ -45,9 +45,7 @@ func CollectSecretReferences(yamlContent string) []string { // Sort for consistent output SortStrings(secrets) - if log.Enabled() { - secretMaskingLog.Printf("Found %d unique secret reference(s) in workflow", len(secrets)) - } + secretMaskingLog.Printf("Found %d unique secret reference(s) in workflow", len(secrets)) return secrets } diff --git a/pkg/workflow/safe_outputs_config.go b/pkg/workflow/safe_outputs_config.go index b4afae62c94..a28950ad368 100644 --- a/pkg/workflow/safe_outputs_config.go +++ b/pkg/workflow/safe_outputs_config.go @@ -18,9 +18,7 @@ func (c *Compiler) extractSafeOutputsConfig(frontmatter map[string]any) *SafeOut if output, exists := frontmatter["safe-outputs"]; exists { if outputMap, ok := output.(map[string]any); ok { - if log.Enabled() { - safeOutputsConfigLog.Printf("Processing safe-outputs configuration with %d top-level keys", len(outputMap)) - } + safeOutputsConfigLog.Printf("Processing safe-outputs configuration with %d top-level keys", len(outputMap)) config = &SafeOutputsConfig{} // Handle create-issue