-
Notifications
You must be signed in to change notification settings - Fork 241
Improve Gemini engine diagnostics: DEBUG env var, error log artifacts, and remove model fallback #17558
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
Improve Gemini engine diagnostics: DEBUG env var, error log artifacts, and remove model fallback #17558
Changes from all commits
99b4325
ae307e1
7a25b1b
dd44056
d1f643c
4dbb7ff
33d8565
59d2c73
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -6,6 +6,7 @@ on: | |||||||
| pull_request: | ||||||||
| types: [labeled] | ||||||||
| names: ["water"] | ||||||||
| reaction: "rocket" | ||||||||
| status-comment: true | ||||||||
| permissions: | ||||||||
| contents: read | ||||||||
|
|
@@ -14,7 +15,6 @@ permissions: | |||||||
| name: Smoke Gemini | ||||||||
| engine: | ||||||||
| id: gemini | ||||||||
|
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. The |
||||||||
| model: gemini-2.0-flash-lite | ||||||||
| strict: true | ||||||||
|
||||||||
| strict: true | |
| model: gemini-2.0-flash-lite | |
| strict: true |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -239,8 +239,21 @@ func TestEngineOutputFileDeclarations(t *testing.T) { | |
| t.Errorf("Codex engine should declare /tmp/gh-aw/mcp-config/logs/, got: %v", codexOutputFiles[0]) | ||
| } | ||
|
|
||
| // Test Gemini engine declares output files for error log collection | ||
| geminiEngine := NewGeminiEngine() | ||
| geminiOutputFiles := geminiEngine.GetDeclaredOutputFiles() | ||
|
|
||
| if len(geminiOutputFiles) == 0 { | ||
| t.Error("Gemini engine should declare output files for error log collection") | ||
| } | ||
|
|
||
| if len(geminiOutputFiles) > 0 && geminiOutputFiles[0] != "/tmp/gemini-client-error-*.json" { | ||
|
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. Good coverage for Gemini-specific error log collection. The glob pattern |
||
| t.Errorf("Gemini engine should declare /tmp/gemini-client-error-*.json, got: %v", geminiOutputFiles[0]) | ||
| } | ||
|
|
||
| t.Logf("Claude engine declares: %v", claudeOutputFiles) | ||
| t.Logf("Codex engine declares: %v", codexOutputFiles) | ||
| t.Logf("Gemini engine declares: %v", geminiOutputFiles) | ||
| } | ||
|
|
||
| func TestEngineOutputCleanupExcludesTmpFiles(t *testing.T) { | ||
|
|
@@ -612,3 +625,59 @@ This workflow tests that the redacted URLs log file is included in artifact uplo | |
|
|
||
| t.Log("Successfully verified that redacted URLs log path is included in engine output collection") | ||
| } | ||
|
|
||
| func TestGeminiEngineOutputFilesGeneratedByCompiler(t *testing.T) { | ||
| // Create temporary directory for test files | ||
| tmpDir := testutil.TempDir(t, "gemini-engine-output-test") | ||
|
|
||
| testContent := `--- | ||
| on: push | ||
| permissions: | ||
| contents: read | ||
| issues: read | ||
| pull-requests: read | ||
| tools: | ||
| github: | ||
| allowed: [list_issues] | ||
| engine: gemini | ||
| --- | ||
|
|
||
| # Test Gemini Engine Output Collection | ||
|
|
||
| This workflow tests that the Gemini engine error log wildcard is uploaded as an artifact. | ||
| ` | ||
|
|
||
| testFile := filepath.Join(tmpDir, "test-gemini-output.md") | ||
| if err := os.WriteFile(testFile, []byte(testContent), 0644); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| compiler := NewCompiler() | ||
| if err := compiler.CompileWorkflow(testFile); err != nil { | ||
| t.Fatalf("Failed to compile Gemini workflow: %v", err) | ||
| } | ||
|
|
||
| lockFile := stringutil.MarkdownToLockFile(testFile) | ||
| lockContent, err := os.ReadFile(lockFile) | ||
| if err != nil { | ||
| t.Fatalf("Failed to read lock file: %v", err) | ||
| } | ||
| lockStr := string(lockContent) | ||
|
|
||
| // Verify that the compiler generates the Upload engine output files step | ||
| if !strings.Contains(lockStr, "- name: Upload engine output files") { | ||
| t.Error("Gemini workflow should have 'Upload engine output files' step generated by compiler") | ||
| } | ||
|
|
||
| // Verify that the Gemini error log wildcard path is included in the artifact upload | ||
| if !strings.Contains(lockStr, "/tmp/gemini-client-error-*.json") { | ||
| t.Error("Gemini workflow should include '/tmp/gemini-client-error-*.json' in engine output artifact upload") | ||
| } | ||
|
|
||
| // Verify that the artifact name is agent_outputs | ||
| if !strings.Contains(lockStr, "name: agent_outputs") { | ||
| t.Error("Gemini engine output artifact should use 'agent_outputs' name") | ||
| } | ||
|
|
||
| t.Log("Successfully verified that Gemini engine error log wildcard is included in engine output collection") | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -176,6 +176,7 @@ func (t *StepOrderTracker) findUnscannablePaths(artifactUploads []StepRecord) [] | |
| } | ||
|
|
||
| // isPathScannedBySecretRedaction checks if a path would be scanned by the secret redaction step | ||
| // or is otherwise safe to upload (known engine-controlled diagnostic paths). | ||
| func isPathScannedBySecretRedaction(path string) bool { | ||
| // Paths must be under /tmp/gh-aw/ or /opt/gh-aw/ to be scanned | ||
| // Accept both literal paths and environment variable references | ||
|
|
@@ -184,7 +185,27 @@ func isPathScannedBySecretRedaction(path string) bool { | |
| // For now, we'll allow ${{ env.* }} patterns through as we can't resolve them at compile time | ||
| // Assume environment variables that might contain /tmp/gh-aw or /opt/gh-aw paths are safe | ||
| // This is a conservative assumption - in practice these are controlled by the compiler | ||
| return strings.Contains(path, "${{ env.") | ||
| if strings.Contains(path, "${{ env.") { | ||
| return true | ||
| } | ||
|
|
||
| // Allow wildcard paths under /tmp/ with a known-safe extension. | ||
| // These are engine-declared diagnostic output files (e.g. Gemini CLI error reports at | ||
| // /tmp/gemini-client-error-*.json). They are produced by the CLI tool itself, not by | ||
| // agent-generated content, and they live outside /tmp/gh-aw/ so they are not scanned by | ||
| // the redact_secrets step. However, these files (JSON error reports, log files) are | ||
| // structurally unlikely to contain raw secret values, so we allow them through validation. | ||
| if strings.HasPrefix(path, "/tmp/") && strings.Contains(path, "*") { | ||
| ext := filepath.Ext(path) | ||
| safeExtensions := []string{".txt", ".json", ".log", ".jsonl"} | ||
| for _, safeExt := range safeExtensions { | ||
| if ext == safeExt { | ||
| return true | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
+192
to
+206
|
||
|
|
||
| return false | ||
| } | ||
|
|
||
| // Path must have one of the scanned extensions: .txt, .json, .log, .jsonl | ||
|
|
||
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.
The addition of
reaction: "rocket"in the workflow frontmatter appears unrelated to the main purpose of this PR (Gemini diagnostics improvements). While this change is valid and generates the expected "Add rocket reaction" step in the lock file, consider whether it should be included in this PR or moved to a separate commit for clarity.