Skip to content

fix: handle errors from os.Setenv and os.Chdir in pkg/cli (10 sites)#33465

Open
Copilot wants to merge 4 commits into
mainfrom
copilot/fix-silent-errors-os-setenv-chdir
Open

fix: handle errors from os.Setenv and os.Chdir in pkg/cli (10 sites)#33465
Copilot wants to merge 4 commits into
mainfrom
copilot/fix-silent-errors-os-setenv-chdir

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 20, 2026

10 production sites in pkg/cli/ silently discarded errors from os.Setenv (4 sites) and os.Chdir (6 sites). The os.Setenv sites handle secrets (Copilot token, system PAT, API key), where a silent failure emits a success message while leaving the env var unset. The os.Chdir sites restore working directory in defers — a silent failure leaves the process in a potentially deleted directory.

Changes

  • engine_secrets.go — Replace _ = os.Setenv(...) with if err + engineSecretsLog.Printf at all 3 secret-storage sites
  • add_interactive_orchestrator.go — Replace bare os.Setenv("GH_HOST", ...) statement with if err + addInteractiveLog.Printf
  • deploy_command.go, update_command.go — Replace _ = os.Chdir(originalDir) inside defer closures with logged error handling using the file's existing logger
  • trial_repository.go (3 sites), trial_helpers.go (1 site) — Convert implicit defer os.Chdir(...) to explicit defer closures with error logging via trialRepoLog / trialLog

All fixes use inline if err patterns with the pre-existing file-scoped loggers — no new packages or helpers added.

// Before
_ = os.Setenv(req.Name, token)

// After
if err := os.Setenv(req.Name, token); err != nil {
    engineSecretsLog.Printf("failed to set env var %s: %v", req.Name, err)
}
// Before
defer os.Chdir(originalDir)

// After
defer func() {
    if err := os.Chdir(originalDir); err != nil {
        trialRepoLog.Printf("failed to chdir back to %s: %v", originalDir, err)
    }
}()

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • https://api.github.com/graphql
    • Triggering command: /usr/bin/gh gh repo view --json owner,name --jq .owner.login + "/" + .name x_amd64/vet GOINSECURE GOMOD GOMODCACHE x_amd64/vet env -json GO111MODULE x_amd64/vet GOINSECURE GOMOD GOMODCACHE x_amd64/vet (http block)
    • Triggering command: /usr/bin/gh gh repo view --json owner,name --jq .owner.login + "/" + .name At,event,headBranch,headSha,displayTitle GOINSECURE GOMOD GOMODCACHE ache/go/1.25.8/x/home/REDACTED/work/gh-aw/gh-aw/pkg/typeutil/convert_test.go (http block)
    • Triggering command: /usr/bin/gh gh pr list --repo github/gh-aw --state all --author app/github-actions --search created:2026-05-11T23:58:00Z..2026-05-12T00:05:00Z --limit 1 --json number --jq .[0].number GOMOD GOMODCACHE ache/go/1.25.8/x--jq (http block)
  • https://api.github.com/orgs/owner/actions/secrets
    • Triggering command: /usr/bin/gh gh api /orgs/owner/actions/secrets --jq .secrets[].name -json GO111MODULE 64/bin/go GOINSECURE 6gzmT9fV37ji GOMODCACHE go env -json GO111MODULE x_amd64/vet GOINSECURE GOMOD GOMODCACHE x_amd64/vet (http block)
  • https://api.github.com/orgs/test-owner/actions/secrets
    • Triggering command: /usr/bin/gh gh api /orgs/test-owner/actions/secrets --jq .secrets[].name -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go env -json GO111MODULE x_amd64/vet GOINSECURE GOMOD GOMODCACHE x_amd64/vet (http block)
  • https://api.github.com/repos/actions/ai-inference/git/ref/tags/v1
    • Triggering command: /usr/bin/gh gh api /repos/actions/ai-inference/git/ref/tags/v1 --jq [.object.sha, .object.type] | @tsv g/cli l /usr/lib/git-core/git -test.timeout=10infocmp -test.run=^Test -test.short=truexterm-color /usr/lib/git-core/git main�� run l ache/go/1.25.8/x64/pkg/tool/linu-nolocalimports --detach GO111MODULE 64/bin/go ache/go/1.25.8/x64/pkg/tool/linu--jq (http block)
  • https://api.github.com/repos/actions/checkout/git/ref/tags/v3
    • Triggering command: /usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v3 --jq [.object.sha, .object.type] | @tsv eutil.go eutil_test.go 64/pkg/tool/linu-f /tmp/go-build653infocmp -trimpath 64/bin/go 64/pkg/tool/linux_amd64/compile -ato�� g_.a faultBranchFromLsRemoteWithRealGitmaster_branch2047586683/002/work ache/go/1.25.8/x64/bin/go -errorsas s -nilfunc /opt/hostedtoolcache/go/1.25.8/xrepos/{owner}/{repo}/actions/runs/4/artifacts (http block)
  • https://api.github.com/repos/actions/checkout/git/ref/tags/v5
    • Triggering command: /usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v5 --jq [.object.sha, .object.type] | @tsv g_.a GO111MODULE ache/go/1.25.8/x64/pkg/tool/linux_amd64/compile GOINSECURE GOMOD GOMODCACHE ache/go/1.25.8/x64/pkg/tool/linustatus -uns�� awRn2yH4Q /tmp/go-build3293093916/b142/vet.cfg ache/go/1.25.8/x64/pkg/tool/linux_amd64/asm GOSUMDB b/gh-aw/pkg/cons-1 64/bin/go ache/go/1.25.8/x64/pkg/tool/linux_amd64/asm (http block)
    • Triggering command: /usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v5 --jq [.object.sha, .object.type] | @tsv run stmain.go ache/go/1.25.8/x64/pkg/tool/linux_amd64/link --detach GO111MODULE 64/bin/go ache/go/1.25.8/x64/pkg/tool/linu--jq -ato�� 3093916/b580/tty.test -buildtags 3093916/b580/importcfg.link -errorsas -ifaceassert layTitle YKpGvUQ8q8uVS/78hxdm0CBnKcsfK4zb--jq (http block)
    • Triggering command: /usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v5 --jq [.object.sha, .object.type] | @tsv /tmp/TestHashStability_SameInputSameOutput3042698293/001/stabilitest-logs/run-12346 x_amd64/vet /usr/lib/git-core/git /workflows GO111MODULE x_amd64/vet /usr/lib/git-core/git main�� run resolved$ /usr/bin/git --detach .cfg 64/pkg/tool/linuxterm-color git (http block)
  • https://api.github.com/repos/actions/checkout/git/ref/tags/v6
    • Triggering command: /usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v6 --jq [.object.sha, .object.type] | @tsv json.test /tmp/go-build3293093916/b005/vet.cfg 64/pkg/tool/linux_amd64/vet GOSUMDB GOWORK 64/bin/go 64/pkg/tool/linux_amd64/vet -ato�� runs/20260520-054736-33393/test-source-field-varremote.upstream.url -buildtags g_.a url -ifaceassert -nilfunc e/git (http block)
    • Triggering command: /usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v6 --jq [.object.sha, .object.type] | @tsv -unreachable=false /tmp/go-build3293093916/b046/vet.cfg 3093916/b281/vet.cfg GOSUMDB GOWORK 64/bin/go /opt/hostedtoolcache/go/1.25.8/xupstream -ato�� ithub-script/git/ref/tags/v9 -buildtags e/git-upload-pack -errorsas -ifaceassert -nilfunc /opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linutest@example.com (http block)
    • Triggering command: /usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v6 --jq [.object.sha, .object.type] | @tsv 4736-33393/test-id,name,path,state /tmp/go-build3293093916/b057/vet.cfg ache/go/1.25.8/x64/pkg/tool/linux_amd64/vet GOSUMDB GOWORK 64/bin/go ache/go/1.25.8/x64/pkg/tool/linuremote.upstream.url -ato�� ab-testing-advisor.md -buildtags flow.lock.yml -errorsas -ifaceassert -nilfunc /opt/hostedtoolcache/go/1.25.8/xTest User (http block)
  • https://api.github.com/repos/actions/github-script/git/ref/tags/v8
    • Triggering command: /usr/bin/gh gh api /repos/actions/github-script/git/ref/tags/v8 --jq [.object.sha, .object.type] | @tsv bility_SameInputSameOutput3042698293/001/stability-test.md {{context.GOARCH}} {{context.Compiler}} /opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linux_amd64/compile unsafe GO111MODULE x_amd64/vet /opt/hostedtoolcache/go/1.25.8/x--jq -o /tmp/go-build3293093916/b545/_pkg_.a -trimpath /usr/bin/gh -p main -lang=go1.25 gh (http block)
    • Triggering command: /usr/bin/gh gh api /repos/actions/github-script/git/ref/tags/v8 --jq [.object.sha, .object.type] | @tsv k/gh-aw/gh-aw/pkg/cli remote.origin.url /usr/bin/git url GO111MODULE x_amd64/vet git conf�� --get remote.origin.url /opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linu-nolocalimports -json GO111MODULE x_amd64/vet /opt/hostedtoolc--jq (http block)
  • https://api.github.com/repos/actions/github-script/git/ref/tags/v9
    • Triggering command: /usr/bin/gh gh api /repos/actions/github-script/git/ref/tags/v9 --jq [.object.sha, .object.type] | @tsv -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go (http block)
    • Triggering command: /usr/bin/gh gh api /repos/actions/github-script/git/ref/tags/v9 --jq [.object.sha, .object.type] | @tsv -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE UvxdjUkN6dwU env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go (http block)
    • Triggering command: /usr/bin/gh gh api /repos/actions/github-script/git/ref/tags/v9 --jq [.object.sha, .object.type] | @tsv -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go env -json GO111MODULE x_amd64/vet GOINSECURE GOMOD GOMODCACHE x_amd64/vet (http block)
  • https://api.github.com/repos/actions/github-script/git/ref/tags/v9.0.0
    • Triggering command: /usr/bin/gh gh api /repos/actions/github-script/git/ref/tags/v9.0.0 --jq [.object.sha, .object.type] | @tsv -t security 64/bin/go OUTPUT -d 168.63.129.16 go env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go (http block)
    • Triggering command: /usr/bin/gh gh api /repos/actions/github-script/git/ref/tags/v9.0.0 --jq [.object.sha, .object.type] | @tsv -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go (http block)
    • Triggering command: /usr/bin/gh gh api /repos/actions/github-script/git/ref/tags/v9.0.0 --jq [.object.sha, .object.type] | @tsv go1.25.8 -c=4 -nolocalimports -importcfg /tmp/go-build3293093916/b492/importcfg -pack /tmp/go-build3293093916/b492/_testmain.go env -json GO111MODULE x_amd64/vet GOINSECURE GOMOD GOMODCACHE x_amd64/vet (http block)
  • https://api.github.com/repos/actions/setup-go/git/ref/tags/v4
    • Triggering command: /usr/bin/gh gh api /repos/actions/setup-go/git/ref/tags/v4 --jq [.object.sha, .object.type] | @tsv -bool l /opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linu-nilfunc 001' 001' -nilfunc /opt/hostedtoolcache/go/1.25.8/xremote -ato�� licyMinIntegrityOnlymin-integrity_with_repos_array_c69691577/001remote.upstream.url -buildtags 3093916/b553/repoutil.test -errorsas -ifaceassert -nilfunc 3093916/b553/rep--jq (http block)
  • https://api.github.com/repos/actions/setup-node/git/ref/tags/v4
    • Triggering command: /usr/bin/gh gh api /repos/actions/setup-node/git/ref/tags/v4 --jq [.object.sha, .object.type] | @tsv 4736-33393/test-3273635340/.github/workflows /tmp/go-build3293093916/b146/vet.cfg ache/go/1.25.8/x64/bin/go GOSUMDB GOWORK 64/bin/go /opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linu-buildtags -uns�� 4736-33393/test-4094695073 /tmp/go-build3293093916/b246/vet.cfg ache/go/1.25.8/x64/pkg/tool/linux_amd64/link -json GO111MODULE 64/bin/go ache/go/1.25.8/x64/pkg/tool/linuremote.origin.url (http block)
    • Triggering command: /usr/bin/gh gh api /repos/actions/setup-node/git/ref/tags/v4 --jq [.object.sha, .object.type] | @tsv 5744136/b001/exe/a.out 3093916/b498/_testmain.go 5744136/b001/importcfg.link -p rdS-1IUMBqVCt/k--1 -lang=go1.25 5744136/b001/_pkg_.a -o licyMinIntegrityOnlymin-integritremote.upstream.url -importcfg ache/go/1.25.8/x64/pkg/tool/linux_amd64/vet url -w -buildmode=exe ache/go/1.25.8/x--jq (http block)
    • Triggering command: /usr/bin/gh gh api /repos/actions/setup-node/git/ref/tags/v4 --jq [.object.sha, .object.type] | @tsv ithub-script/git/ref/tags/v9 --jq bject.type] | @tsv -json GO111MODULE 64/bin/go gh api /repos/actions/github-script/git/ref/tags/v9 (http block)
  • https://api.github.com/repos/actions/setup-node/git/ref/tags/v6
    • Triggering command: /usr/bin/gh gh api /repos/actions/setup-node/git/ref/tags/v6 --jq [.object.sha, .object.type] | @tsv verutil.go verutil_test.go 64/pkg/tool/linux_amd64/compile GOSUMDB GOWORK 64/bin/go 64/pkg/tool/linux_amd64/compile -ato�� 2847799830 -buildtags e/git -errorsas -ifaceassert -nilfunc e/git (http block)
    • Triggering command: /usr/bin/gh gh api /repos/actions/setup-node/git/ref/tags/v6 --jq [.object.sha, .object.type] | @tsv /001/inlined-b.md /tmp/go-build3293093916/b045/vet.cfg 3093916/b317/vet.cfg GOSUMDB GOWORK 64/bin/go /opt/hostedtoolcache/go/1.25.8/xupstream -ato�� 2847799830 -buildtags 3093916/b434/vet.cfg -errorsas -ifaceassert -nilfunc /opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linu-tests (http block)
    • Triggering command: /usr/bin/gh gh api /repos/actions/setup-node/git/ref/tags/v6 --jq [.object.sha, .object.type] | @tsv -unreachable=false /tmp/go-build3293093916/b059/vet.cfg ache/go/1.25.8/x64/pkg/tool/linux_amd64/vet GOSUMDB GOWORK 64/bin/go NY/es83SkSH2yExAhqTyUHF/pN7cv-DOVeUyFlVlSS3Z -ato�� ace-editor.md -buildtags 1/x64/bin/node -errorsas -ifaceassert -nilfunc /opt/hostedtoolcache/go/1.25.8/xremote.origin.url (http block)
  • https://api.github.com/repos/actions/upload-artifact/git/ref/tags/v4
    • Triggering command: /usr/bin/gh gh api /repos/actions/upload-artifact/git/ref/tags/v4 --jq [.object.sha, .object.type] | @tsv -unreachable=faldiagnostic noise should not be returned /tmp/go-build3293093916/b058/vet.cfg 3093916/b335/vet.cfg GOSUMDB GOWORK 64/bin/go /opt/hostedtoolcache/go/1.25.8/xorigin -ato�� agent-performance-analyzer.md (http block)
  • https://api.github.com/repos/aws-actions/configure-aws-credentials/git/ref/tags/v4
    • Triggering command: /usr/bin/gh gh api /repos/aws-actions/configure-aws-credentials/git/ref/tags/v4 --jq [.object.sha, .object.type] | @tsv /orgs/test-owner/actions/secrets --jq /usr/bin/infocmp -json GO111MODULE 64/bin/go infocmp -1 xterm-color go ache/node/24.14.1/x64/bin/node -json GO111MODULE x_amd64/vet git (http block)
    • Triggering command: /usr/bin/gh gh api /repos/aws-actions/configure-aws-credentials/git/ref/tags/v4 --jq [.object.sha, .object.type] | @tsv ripts/lint_error_messages.go ripts/lint_error_messages_test.go ache/go/1.25.8/x64/pkg/tool/linux_amd64/compile -json GO111MODULE x_amd64/vet ache/go/1.25.8/x64/pkg/tool/linu--jq -C 3093916/b593/_pkg_.a config 3093916/b593=> remote.upstream.git GO111MODULE x_amd64/vet infocmp (http block)
  • https://api.github.com/repos/azure/login/git/ref/tags/v2
    • Triggering command: /usr/bin/gh gh api /repos/azure/login/git/ref/tags/v2 --jq [.object.sha, .object.type] | @tsv /tmp/go-build3293093916/b528/excessivefuncparams.test resolved$ /opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linux_amd64/vet -s -w -buildmode=exe /opt/hostedtoolcache/go/1.25.8/x--jq -ato�� v1.0.0 -buildtags /usr/bin/git -errorsas -ifaceassert -nilfunc git (http block)
  • https://api.github.com/repos/docker/login-action/git/ref/tags/v3
    • Triggering command: /usr/bin/gh gh api /repos/docker/login-action/git/ref/tags/v3 --jq [.object.sha, .object.type] | @tsv ts.TOKEN ^remote\..*\.gh-resolved$ /opt/hostedtoolcache/go/1.25.8/x64/pkg/tool/linux_amd64/compile -json GO111MODULE 64/bin/go /opt/hostedtoolcache/go/1.25.8/x--jq -V=f�� GOMODCACHE go 64/pkg/tool/linux_amd64/compile -json GO111MODULE x_amd64/vet 64/pkg/tool/linux_amd64/compile (http block)
  • https://api.github.com/repos/github/gh-aw-actions/git/ref/tags/v0.1.2
    • Triggering command: /usr/bin/gh gh api /repos/github/gh-aw-actions/git/ref/tags/v0.1.2 --jq [.object.sha, .object.type] | @tsv /v2.0.0 stmain.go r: $owner, name: $name) { hasDiscussionsEnabled } } -errorsas -ifaceassert -nilfunc /opt/hostedtoolcache/go/1.25.8/x--jq -o /tmp/go-build3293093916/b478/cli.test url ache/go/1.25.8/x64/pkg/tool/linux_amd64/vet -s -w -buildmode=exe ache/go/1.25.8/x--jq (http block)
  • https://api.github.com/repos/github/gh-aw-actions/git/ref/tags/v1.0.0
    • Triggering command: /usr/bin/gh gh api /repos/github/gh-aw-actions/git/ref/tags/v1.0.0 --jq [.object.sha, .object.type] | @tsv EB2bs7aww /tmp/go-build329.owner.login + "/" + .name tartedAt,updatedAt,event,headBranch,headSha,displayTitle GOSUMDB GOWORK 64/bin/go /opt/hostedtoolc--jq ortc�� artifacts-summary.md stmain.go 1/x64/bin/node /tmp/go-build653infocmp -trimpath 64/bin/go ache/go/1.25.8/x64/pkg/tool/linux_amd64/link (http block)
  • https://api.github.com/repos/github/gh-aw-actions/git/ref/tags/v1.2.3
    • Triggering command: /usr/bin/gh gh api /repos/github/gh-aw-actions/git/ref/tags/v1.2.3 --jq [.object.sha, .object.type] | @tsv se piler}} tartedAt,updatedAt,event,headBranch,headSha,displayTitle - GOWORK 64/bin/go /opt/hostedtoolcconfig -uns�� archie.md /tmp/go-build3293093916/b219/vet.cfg 64/pkg/tool/linux_amd64/link /tmp/go-build653infocmp -trimpath 64/bin/go 64/pkg/tool/linux_amd64/link (http block)
  • https://api.github.com/repos/github/gh-aw/actions/runs
    • Triggering command: /usr/bin/gh gh run list --json databaseId,number,url,status,conclusion,workflowName,createdAt,startedAt,updatedAt,event,headBranch,headSha,displayTitle --limit 100 --created >=2026-05-13 GOMOD GOMODCACHE 64/pkg/tool/linuTest User env -json GO111MODULE x_amd64/compile GOINSECURE GOMOD GOMODCACHE x_amd64/compile (http block)
    • Triggering command: /usr/bin/gh gh run list --json databaseId,number,url,status,conclusion,workflowName,createdAt,startedAt,updatedAt,event,headBranch,headSha,displayTitle --limit 100 --created >=2026-04-20 GOMOD GOMODCACHE 64/pkg/tool/linuremote.origin.url env -json GO111MODULE 64/pkg/tool/linux_amd64/vet GOINSECURE GOMOD GOMODCACHE 64/pkg/tool/linux_amd64/vet (http block)
    • Triggering command: /usr/bin/gh gh run list --json databaseId,number,url,status,conclusion,workflowName,createdAt,startedAt,updatedAt,event,headBranch,headSha,displayTitle --limit 100 --created >=2026-02-19 GOMOD GOMODCACHE 64/pkg/tool/linuremote.origin.url env -json GO111MODULE 64/pkg/tool/linux_amd64/vet GOINSECURE GOMOD GOMODCACHE 64/pkg/tool/linux_amd64/vet (http block)
  • https://api.github.com/repos/github/gh-aw/actions/runs/1/artifacts
    • Triggering command: /usr/bin/gh gh api --paginate repos/{owner}/{repo}/actions/runs/1/artifacts --jq .artifacts[].name .cfg .cfg GOINSECURE GOMOD GOMODCACHE ache/go/1.25.8/xother (http block)
    • Triggering command: /usr/bin/gh gh run download 1 --dir test-logs/run-1 3093916/b036/vet.cfg x_amd64/compile GOINSECURE GOMOD GOMODCACHE x_amd64/compile 2930�� se 3093916/b083/vet.cfg .cfg GOINSECURE GOMOD GOMODCACHE ache/go/1.25.8/xtest@example.com (http block)
  • https://api.github.com/repos/github/gh-aw/actions/runs/12345/artifacts
    • Triggering command: /usr/bin/gh gh api --paginate repos/{owner}/{repo}/actions/runs/12345/artifacts --jq .artifacts[].name .cfg .test GOINSECURE GOMOD GOMODCACHE .test 2930�� se 3093916/b079/vet.cfg ache/go/1.25.8/x64/pkg/tool/linux_amd64/vet GOINSECURE GOMOD GOMODCACHE ache/go/1.25.8/x64/pkg/tool/linux_amd64/vet (http block)
    • Triggering command: /usr/bin/gh gh run download 12345 --dir test-logs/run-12345 GO111MODULE 64/pkg/tool/linux_amd64/vet GOINSECURE b/gh-aw/actions/config GOMODCACHE yLNKNaz/um86pbnuremote.origin.url (http block)
  • https://api.github.com/repos/github/gh-aw/actions/runs/1234567890
    • Triggering command: /usr/bin/gh gh api repos/{owner}/{repo}/actions/runs/1234567890 --jq {databaseId: .id, number: .run_number, url: .html_url, status: .status, conclusion: .conclusion, workflowName: .name, workflowPath: .path, createdAt: .created_at, startedAt: .run_started_at, updatedAt: .updated_at, event: .event, headBranch: .head_branch, -json GO111MODULE x_amd64/vet GOINSECURE GOMOD GOMODCACHE x_amd64/vet env -json GO111MODULE x_amd64/vet GOINSECURE GOMOD GOMODCACHE x_amd64/vet (http block)
  • https://api.github.com/repos/github/gh-aw/actions/runs/12346/artifacts
    • Triggering command: /usr/bin/gh gh api --paginate repos/{owner}/{repo}/actions/runs/12346/artifacts --jq .artifacts[].name GO111MODULE 64/pkg/tool/linux_amd64/vet GOINSECURE GOMOD GOMODCACHE 64/pkg/tool/linux_amd64/vet (http block)
    • Triggering command: /usr/bin/gh gh run download 12346 --dir test-logs/run-12346 3093916/b009/vet.cfg orutil.test GOINSECURE GOMOD GOMODCACHE orutil.test 2930�� y_with_repos=public_3347312900/0remote.origin.url 3093916/b095/vet.cfg ache/go/1.25.8/x64/pkg/tool/linux_amd64/vet GOINSECURE GOMOD GOMODCACHE ache/go/1.25.8/xTest User (http block)
  • https://api.github.com/repos/github/gh-aw/actions/runs/2/artifacts
    • Triggering command: /usr/bin/gh gh api --paginate repos/{owner}/{repo}/actions/runs/2/artifacts --jq .artifacts[].name l_test.go 64/pkg/tool/linux_amd64/compile GOINSECURE GOMOD GOMODCACHE 64/pkg/tool/linux_amd64/compile epOn�� ut676624266/001 3093916/b092/vet.cfg 64/pkg/tool/linux_amd64/compile GOINSECURE GOMOD GOMODCACHE 64/pkg/tool/linux_amd64/compile (http block)
    • Triggering command: /usr/bin/gh gh run download 2 --dir test-logs/run-2 3093916/b040/vet.cfg .cfg GOINSECURE GOMOD GOMODCACHE ache/go/1.25.8/xremote.origin.url epOn�� 0612832/001 GO111MODULE .cfg GOINSECURE GOMOD GOMODCACHE ache/go/1.25.8/xTest User (http block)
  • https://api.github.com/repos/github/gh-aw/actions/runs/3/artifacts
    • Triggering command: /usr/bin/gh gh api --paginate repos/{owner}/{repo}/actions/runs/3/artifacts --jq .artifacts[].name .cfg .cfg GOINSECURE GOMOD GOMODCACHE ache/go/1.25.8/xmyorg (http block)
    • Triggering command: /usr/bin/gh gh run download 3 --dir test-logs/run-3 3093916/b041/vet.cfg .cfg GOINSECURE GOMOD GOMODCACHE ache/go/1.25.8/xremote2 env edOutput1868768900/001 GO111MODULE k GOINSECURE GOMOD GOMODCACHE ache/go/1.25.8/xremote.origin.url (http block)
  • https://api.github.com/repos/github/gh-aw/actions/runs/4/artifacts
    • Triggering command: /usr/bin/gh gh api --paginate repos/{owner}/{repo}/actions/runs/4/artifacts --jq .artifacts[].name oFiles,IgnoredOtherFiles,CFiles,CgoFiles,CXXFiles,MFiles,HFiles,FFiles,SFiles,SwigFiles,SwigCXXFconfig 64/pkg/tool/linux_amd64/compile GOINSECURE GOMOD GOMODCACHE 64/pkg/tool/linux_amd64/compile (http block)
    • Triggering command: /usr/bin/gh gh run download 4 --dir test-logs/run-4 3093916/b042/vet.cfg .cfg GOINSECURE GOMOD GOMODCACHE ache/go/1.25.8/xremote1 env edOutput1868768900/001 GO111MODULE .cfg GOINSECURE GOMOD GOMODCACHE ache/go/1.25.8/x64/pkg/tool/linu--jq (http block)
  • https://api.github.com/repos/github/gh-aw/actions/runs/5/artifacts
    • Triggering command: /usr/bin/gh gh api --paginate repos/{owner}/{repo}/actions/runs/5/artifacts --jq .artifacts[].name .cfg 64/pkg/tool/linux_amd64/link GOINSECURE GOMOD GOMODCACHE 64/pkg/tool/linux_amd64/link (http block)
    • Triggering command: /usr/bin/gh gh run download 5 --dir test-logs/run-5 3093916/b027/vet.cfg 64/pkg/tool/linux_amd64/link GOINSECURE GOMOD GOMODCACHE 64/pkg/tool/linuremote2 env 0612832/001 GO111MODULE ortcfg.link GOINSECURE GOMOD GOMODCACHE 31jFQ5S5Hbd2a1KlD8/98TAUgI09TyYA--jq (http block)
  • https://api.github.com/repos/github/gh-aw/actions/workflows
    • Triggering command: /usr/bin/gh gh workflow list --json name,state,path -json (http block)
    • Triggering command: /usr/bin/gh gh run list --json databaseId,number,url,status,conclusion,workflowName,createdAt,startedAt,updatedAt,event,headBranch,headSha,displayTitle --workflow nonexistent-workflow-12345 --limit 100 GOMOD GOMODCACHE x_amd64/vet env -json GO111MODULE x_amd64/vet GOINSECURE GOMOD GOMODCACHE x_amd64/vet (http block)
    • Triggering command: /usr/bin/gh gh run list --json databaseId,number,url,status,conclusion,workflowName,createdAt,startedAt,updatedAt,event,headBranch,headSha,displayTitle --workflow nonexistent-workflow-12345 --limit 6 GOMOD GOMODCACHE ntdrain.test 2930�� 3141200549/.github/workflows GO111MODULE .cfg GOINSECURE GOMOD GOMODCACHE ache/go/1.25.8/x--json (http block)
  • https://api.github.com/repos/github/gh-aw/contents/.github/workflows/shared/reporting.md
    • Triggering command: /tmp/go-build3293093916/b478/cli.test /tmp/go-build3293093916/b478/cli.test -test.testlogfile=/tmp/go-build3293093916/b478/testlog.txt -test.paniconexit0 -test.v=true -test.parallel=4 -test.timeout=10m0s -test.run=^Test -test.short=true GOINSECURE GOMOD GOMODCACHE go env -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go (http block)
  • https://api.github.com/repos/github/gh-aw/git/ref/tags/dev
    • Triggering command: /usr/bin/gh gh api /repos/github/gh-aw/git/ref/tags/dev --jq [.object.sha, .object.type] | @tsv /ref/tags/v9.0.0 go sv -json GO111MODULE x_amd64/vet git conf�� runs/20260520-054736-33393/test-2847799830 git /usr/bin/git -json GO111MODULE x_amd64/vet git (http block)
    • Triggering command: /usr/bin/gh gh api /repos/github/gh-aw/git/ref/tags/dev --jq [.object.sha, .object.type] | @tsv sistency_InlinedImports780653626/001/inlined-b.md remote /usr/bin/git -json GO111MODULE x_amd64/vet git remo�� runs/20260520-054736-33393/test-2847799830 x_amd64/vet ache/node/24.14.1/x64/bin/node -json GO111MODULE x_amd64/vet ache/node/24.14.1/x64/bin/node (http block)
  • https://api.github.com/repos/github/gh-aw/git/ref/tags/v0.47.4
    • Triggering command: /usr/bin/gh gh api /repos/github/gh-aw/git/ref/tags/v0.47.4 --jq [.object.sha, .object.type] | @tsv bility_SameInputSameOutput3042698293/001/stability-test.md go /usr/bin/git -json GO111MODULE x_amd64/vet git -C /tmp/TestGuardPolicyBlockedUsersApprovalLabelsCompiledOutput3457466613/001 (http block)
  • https://api.github.com/repos/github/gh-aw/git/ref/tags/v1.0.0
    • Triggering command: /usr/bin/gh gh api /repos/github/gh-aw/git/ref/tags/v1.0.0 --jq [.object.sha, .object.type] | @tsv sRCHwAD-i 3093916/b241/vet.cfg 64/pkg/tool/linux_amd64/compile GOINSECURE GOMOD GOMODCACHE 64/pkg/tool/linux_amd64/compile ortc�� _.a stmain.go x_amd64/vet GOINSECURE GOMOD GOMODCACHE x_amd64/vet (http block)
  • https://api.github.com/repos/github/gh-aw/git/ref/tags/v1.2.3
    • Triggering command: /usr/bin/gh gh api /repos/github/gh-aw/git/ref/tags/v1.2.3 --jq [.object.sha, .object.type] | @tsv -json GO111MODULE x_amd64/vet GOINSECURE GOMOD GOMODCACHE x_amd64/vet env -json .cfg 64/pkg/tool/linux_amd64/vet GOINSECURE GOMOD GOMODCACHE 64/pkg/tool/linux_amd64/vet (http block)
  • https://api.github.com/repos/github/gh-aw/git/ref/tags/v2.0.0
    • Triggering command: /usr/bin/gh gh api /repos/github/gh-aw/git/ref/tags/v2.0.0 --jq [.object.sha, .object.type] | @tsv -json GO111MODULE x_amd64/vet GOINSECURE GOMOD GOMODCACHE mDRmydbNI-g3 env -json .cfg 64/pkg/tool/linu-nolocalimports GOINSECURE GOMOD GOMODCACHE VJO39ky/GOMoRSmSconfig (http block)
    • Triggering command: /usr/bin/gh gh api /repos/github/gh-aw/git/ref/tags/v2.0.0 --jq [.object.sha, .object.type] | @tsv -json GO111MODULE x_amd64/vet GOINSECURE GOMOD GOMODCACHE x_amd64/vet env ub/workflows .cfg 64/pkg/tool/linux_amd64/vet GOINSECURE GOMOD GOMODCACHE 64/pkg/tool/linushow (http block)
    • Triggering command: /usr/bin/gh gh api /repos/github/gh-aw/git/ref/tags/v2.0.0 --jq [.object.sha, .object.type] | @tsv -json GO111MODULE x_amd64/vet GOINSECURE GOMOD GOMODCACHE x_amd64/vet env -json .cfg 64/pkg/tool/linux_amd64/vet GOINSECURE GOMOD GOMODCACHE 64/pkg/tool/linu-buildtags (http block)
  • https://api.github.com/repos/github/gh-aw/git/ref/tags/v3.0.0
    • Triggering command: /usr/bin/gh gh api /repos/github/gh-aw/git/ref/tags/v3.0.0 --jq [.object.sha, .object.type] | @tsv -json GO111MODULE x_amd64/vet GOINSECURE GOMOD GOMODCACHE x_amd64/vet env b/workflows .cfg x_amd64/vet GOINSECURE GOMOD GOMODCACHE x_amd64/vet (http block)
  • https://api.github.com/repos/github/gh-aw/issues/17
    • Triggering command: /usr/bin/gh gh api repos/github/gh-aw/issues/17 go env -json GO111MODULE x_amd64/vet GOINSECURE GOMOD GOMODCACHE x_amd64/vet env -json GO111MODULE x_amd64/vet GOINSECURE GOMOD GOMODCACHE x_amd64/vet (http block)
  • https://api.github.com/repos/google-github-actions/auth/git/ref/tags/v2
    • Triggering command: /usr/bin/gh gh api /repos/google-github-actions/auth/git/ref/tags/v2 --jq [.object.sha, .object.type] | @tsv list l /usr/bin/git -json (http block)
  • https://api.github.com/repos/nonexistent/action/git/ref/tags/v999.999.999
    • Triggering command: /usr/bin/gh gh api /repos/nonexistent/action/git/ref/tags/v999.999.999 --jq [.object.sha, .object.type] | @tsv 4094695073 3093916/b244/vet.cfg 64/pkg/tool/linux_amd64/link GOINSECURE GOMOD GOMODCACHE 64/pkg/tool/linuremote.upstream.url env 42/001/test-inlined-imports-enabled-with-body-content.md GO111MODULE ache/go/1.25.8/x64/pkg/tool/linux_amd64/compile GOINSECURE GOMOD GOMODCACHE ache/go/1.25.8/xremote.upstream.url (http block)
  • https://api.github.com/repos/nonexistent/repo/actions/runs/12345
    • Triggering command: /usr/bin/gh gh run view 12345 --repo nonexistent/repo --json status,conclusion GOINSECURE onpins GOMODCACHE ache/go/1.25.8/xmyorg (http block)
  • https://api.github.com/repos/org/repo/pulls/1
    • Triggering command: /usr/bin/gh gh api repos/org/repo/pulls/1 AvrYP13gIHUH env -json GO111MODULE x_amd64/vet GOINSECURE GOMOD GOMODCACHE x_amd64/vet env -json GO111MODULE x_amd64/vet GOINSECURE GOMOD GOMODCACHE x_amd64/vet (http block)
  • https://api.github.com/repos/owner/repo/actions/secrets
    • Triggering command: /usr/bin/gh gh api /repos/owner/repo/actions/secrets --jq .secrets[].name -json GO111MODULE 64/bin/go GOINSECURE GOMOD GOMODCACHE go env -json GO111MODULE x_amd64/vet GOINSECURE GOMOD GOMODCACHE x_amd64/vet (http block)
  • https://api.github.com/repos/owner/repo/actions/workflows
    • Triggering command: /usr/bin/gh gh workflow list --json name,state,path --repo owner/repo x_amd64/asm GOINSECURE GOMOD GOMODCACHE x_amd64/asm env -json GO111MODULE x_amd64/vet GOINSECURE GOMOD GOMODCACHE x_amd64/vet (http block)
    • Triggering command: /usr/bin/gh gh workflow list --json name,state,path --repo owner/repo x_amd64/vet GOINSECURE GOMOD GOMODCACHE x_amd64/vet env -json GO111MODULE x_amd64/vet GOINSECURE GOMOD GOMODCACHE x_amd64/vet (http block)
    • Triggering command: /usr/bin/gh gh run list --json databaseId,number,url,status,conclusion,workflowName,createdAt,startedAt,updatedAt,event,headBranch,headSha,displayTitle --workflow nonexistent-remote-workflow --limit 30 --repo owner/repo 64/pkg/tool/linux_amd64/compile (http block)
  • https://api.github.com/repos/test-owner/test-repo/actions/secrets
    • Triggering command: /usr/bin/gh gh api /repos/test-owner/test-repo/actions/secrets --jq .secrets[].name go1.25.8 -c=4 -nolocalimports -importcfg /tmp/go-build3293093916/b522/importcfg -pack /tmp/go-build3293093916/b522/_testmain.go env -json GO111MODULE x_amd64/vet GOINSECURE GOMOD GOMODCACHE x_amd64/vet (http block)
  • https://api.github.com/repos/test/repo
    • Triggering command: /usr/bin/gh gh api /repos/test/repo --jq .default_branch y.md 3093916/b225/vet.cfg 64/pkg/tool/linux_amd64/link GOINSECURE GOMOD GOMODCACHE 64/pkg/tool/linux_amd64/link env 42/001/test-frontmatter-with-nested-objects.md GO111MODULE ortcfg.link GOINSECURE GOMOD GOMODCACHE u-pnaIdGhEB2bs7aww/46Oo2oR4WeNsdorigin (http block)

If you need me to access, download, or install something from one of these locations, you can either:

- add_interactive_orchestrator.go: log warning if os.Setenv("GH_HOST") fails
- engine_secrets.go: log warning if os.Setenv fails for Copilot token, system PAT, generic API key
- deploy_command.go: log warning if os.Chdir(originalDir) fails in defer
- update_command.go: log warning if os.Chdir(originalDir) fails in defer
- trial_repository.go: log warning if os.Chdir fails in defer (3 sites)
- trial_helpers.go: log warning if os.Chdir(originalDir) fails in defer

Fixes #<issue-number>"

Agent-Logs-Url: https://github.com/github/gh-aw/sessions/6165c152-eaf7-4169-86aa-14a9237c8ff6

Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix silent error discards on os.Setenv and os.Chdir in pkg/cli fix: handle errors from os.Setenv and os.Chdir in pkg/cli (10 sites) May 20, 2026
Copilot AI requested a review from gh-aw-bot May 20, 2026 05:51
@pelikhan pelikhan marked this pull request as ready for review May 20, 2026 06:12
Copilot AI review requested due to automatic review settings May 20, 2026 06:12
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 20, 2026

🧪 Test Quality Sentinel completed test quality analysis.

No test files were added or modified in PR #33465. This PR only adds error handling for os.Setenv and os.Chdir calls in 6 production files (pkg/cli/*.go) without accompanying test changes. Test Quality Sentinel analysis skipped.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 20, 2026

PR Code Quality Reviewer completed the code quality review.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 20, 2026

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates pkg/cli to stop discarding errors returned by os.Setenv (for secret/host configuration) and os.Chdir (when restoring the working directory in defers), by checking errors and logging failures.

Changes:

  • Add if err := os.Setenv(...); err != nil { ... } for secret storage and GHES host detection.
  • Wrap deferred os.Chdir(...) calls in closures that check and log restore failures.
  • Apply the above across deploy/update/trial flows to improve diagnosability of environment and directory state issues.
Show a summary per file
File Description
pkg/cli/engine_secrets.go Adds error handling for os.Setenv when storing prompted secrets.
pkg/cli/add_interactive_orchestrator.go Adds error handling for os.Setenv("GH_HOST", ...) during GHES auto-detection.
pkg/cli/deploy_command.go Checks/logs errors when restoring original working directory in a defer.
pkg/cli/update_command.go Checks/logs errors when restoring original working directory in a defer.
pkg/cli/trial_repository.go Converts defer os.Chdir(...) to closures with error logging at multiple sites.
pkg/cli/trial_helpers.go Converts defer os.Chdir(...) to a closure with error logging.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comments suppressed due to low confidence (4)

pkg/cli/engine_secrets.go:383

  • If os.Setenv fails here, the code continues and prints a success message, and when config.RepoSlug == "" there’s no fallback persistence for the token. Since engineSecretsLog is debug-only, this can still look like a successful configuration to the user. Consider returning an error (or printing a warning to stderr and not claiming success) when os.Setenv fails.
	// Store in environment for later use
	if err := os.Setenv(req.Name, token); err != nil {
		engineSecretsLog.Printf("failed to set env var %s: %v", req.Name, err)
	}
	fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(req.Name+" token received"))

pkg/cli/engine_secrets.go:438

  • Same issue as the other secret prompts: os.Setenv failure is only logged via a debug logger, but the function proceeds and prints a success message. If config.RepoSlug is empty, the API key won’t be stored anywhere. Consider treating os.Setenv failure as an error (or at least a user-visible warning) to avoid false success.
	// Store in environment for later use
	if err := os.Setenv(req.Name, apiKey); err != nil {
		engineSecretsLog.Printf("failed to set env var %s: %v", req.Name, err)
	}
	fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(label+" API key received"))

pkg/cli/trial_repository.go:238

  • Same as the outer defer: this restoration failure is only logged via a debug logger. Consider printing a user-visible warning to stderr (maybe behind opts.Verbose) so the failure isn’t effectively swallowed.
			defer func() {
				if err := os.Chdir(tempDir); err != nil {
					trialRepoLog.Printf("failed to chdir back to %s: %v", tempDir, err)
				}
			}()

pkg/cli/trial_repository.go:552

  • Failure to chdir back is only recorded via trialRepoLog (debug-only). Consider making this observable to end users (stderr warning, optionally behind verbose) since a stuck working directory can have confusing follow-on effects.
	defer func() {
		if err := os.Chdir(originalDir); err != nil {
			trialRepoLog.Printf("failed to chdir back to %s: %v", originalDir, err)
		}
	}()
  • Files reviewed: 6/6 changed files
  • Comments generated: 6

Comment thread pkg/cli/engine_secrets.go Outdated
// Store in environment for later use
_ = os.Setenv(req.Name, token)
if err := os.Setenv(req.Name, token); err != nil {
engineSecretsLog.Printf("failed to set env var %s: %v", req.Name, err)
addInteractiveLog.Printf("Auto-detected GHES host from git remote: %s", detectedHost)
os.Setenv("GH_HOST", detectedHost)
if err := os.Setenv("GH_HOST", detectedHost); err != nil {
addInteractiveLog.Printf("failed to set GH_HOST env var: %v", err)
Comment thread pkg/cli/deploy_command.go
defer func() {
_ = os.Chdir(originalDir)
if err := os.Chdir(originalDir); err != nil {
deployLog.Printf("failed to chdir back to %s: %v", originalDir, err)
Comment thread pkg/cli/update_command.go
defer func() {
_ = os.Chdir(originalDir)
if err := os.Chdir(originalDir); err != nil {
updateLog.Printf("failed to chdir back to %s: %v", originalDir, err)
Comment on lines +211 to +215
defer func() {
if err := os.Chdir(originalDir); err != nil {
trialRepoLog.Printf("failed to chdir back to %s: %v", originalDir, err)
}
}()
Comment thread pkg/cli/trial_helpers.go
defer os.Chdir(originalDir)
defer func() {
if err := os.Chdir(originalDir); err != nil {
trialLog.Printf("failed to chdir back to %s: %v", originalDir, err)
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

This PR correctly adds error handling for previously ignored os.Setenv and os.Chdir errors. The defer patterns are well-implemented, and logging is appropriate.

Key observations:

  • ✅ Proper error capture in defer closures
  • ✅ Consistent logging approach across all callsites
  • ⚠️ UX gap: Success messages print even when os.Setenv fails for secrets
  • ⚠️ Auto-detected GH_HOST failures have no user-visible warning

Overall: Significant improvement over silently ignoring errors. The remaining UX issues are non-blocking but worth considering in follow-up work.

Note

🔒 Integrity filter blocked 1 item

The following item was blocked because it doesn't meet the GitHub integrity level.

  • #33465 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".

To allow these resources, lower min-integrity in your GitHub frontmatter:

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

🔎 Code quality review by PR Code Quality Reviewer · ● 1.8M

@github-actions github-actions Bot mentioned this pull request May 20, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 20, 2026

PR Code Quality Reviewer completed the code quality review.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 20, 2026

🧪 Test Quality Sentinel completed test quality analysis.

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skills-Based Review 🧠

Applied /diagnose and /tdd based on this bug fix PR addressing silent error handling failures.


Key Themes

1. Incomplete error handling strategy (/diagnose)

The fix addresses the symptom (errors were being silently discarded) but creates a new problem: errors are now logged but execution continues, leading to misleading success messages.

  • engine_secrets.go (lines 329, 379, 434): When os.Setenv fails setting a secret token, the code logs the error but then prints "Valid token received" or "token received". The agent session will fail later with a confusing "missing credential" error far from the root cause.
  • trial_helpers.go, trial_repository.go, deploy_command.go, update_command.go: When os.Chdir fails in a defer, the function returns success but leaves the process in an unexpected directory state.

Root cause analysis needed: The /diagnose skill emphasizes understanding when these errors can actually occur. os.Setenv and os.Chdir rarely fail in normal operation, but when they do (e.g., deleted directory, permission issues, OS resource exhaustion), the failure is significant and should not be silently absorbed.

2. Missing regression tests (/tdd)

No tests verify the new error handling behavior. Without tests:

  • Future refactors might accidentally reintroduce silent error discard
  • The continue-vs-fail decision for each site is undocumented in code
  • The actual failure scenarios are not validated

The /tdd skill's red-green-refactor loop would start with failing tests that reproduce the original bug (silent failures), then verify the fix prevents future regressions.

3. Pattern inconsistency (/diagnose)

The error handling strategy differs across sites without clear justification:

  • Some sites continue on error (GH_HOST detection)
  • Some sites are in defers (directory restoration)
  • Some sites are critical (secret tokens)

This inconsistency suggests the fix applied a uniform pattern without diagnosing which errors are recoverable vs. fatal.


Positive Highlights

  • Consistent error logging: All sites use the appropriate logger with context
  • Proper defer error capture: Anonymous functions correctly capture and check errors
  • Clear commit message: The PR description documents all 10 sites and their context

Recommended Next Steps

  1. Decide error severity for each class of failure:

    • Secrets (os.Setenv for tokens): Should be fatal — return error instead of logging
    • Directory restoration (os.Chdir in defer): Should be captured and surfaced — see inline comment for pattern
    • Feature detection (GH_HOST): Log and continue is appropriate
  2. Add regression tests for at least:

    • Secret token os.Setenv failure (verify error is returned, not logged)
    • Directory restoration failure (verify error is captured in return value)
  3. Update success messages: Don't print success when error handling suppressed a failure


Verdict

Requesting changes because:

  • The engine_secrets.go sites create misleading success messages when errors occur
  • The os.Chdir sites in defers silently fail without surfacing the error to callers
  • No regression tests to prevent re-introduction of silent failures

The fix is close — it correctly identifies the problem sites and adds error checking — but needs refinement to ensure errors are handled appropriately based on their severity.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 4.3M

Comment thread pkg/cli/engine_secrets.go

// Store in environment for later use
_ = os.Setenv(req.Name, token)
if err := os.Setenv(req.Name, token); err != nil {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnose] This fix logs the error but continues execution, printing a success message even when os.Setenv fails. The user sees "Valid fine-grained Copilot token received" but the token is not actually set in the environment.

Impact: The agent session will fail later when trying to use the unset token, with a confusing error message far from the root cause.

Suggestion: Either return the error immediately, or set a flag to skip the success message:

if err := os.Setenv(req.Name, token); err != nil {
    return fmt.Errorf("failed to set %s environment variable: %w", req.Name, err)
}
fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Valid fine-grained Copilot token received"))

Comment thread pkg/cli/engine_secrets.go

// Store in environment for later use
_ = os.Setenv(req.Name, token)
if err := os.Setenv(req.Name, token); err != nil {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnose] Same issue as line 329 — error is logged but execution continues with a misleading success message. The three os.Setenv sites in this file (lines 329, 379, 434) all have the same defect.

Root cause question: Under what conditions does os.Setenv actually fail? Understanding this will inform whether logging is sufficient or if the error should be fatal.

Comment thread pkg/cli/trial_helpers.go
return fmt.Errorf("failed to get current directory: %w", err)
}
defer os.Chdir(originalDir)
defer func() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnose] Defer with error logging is an improvement, but the function still returns successfully even if the chdir back fails.

Scenario: If originalDir was deleted during copyTrialResultsToHostRepo, the defer fails but the function returns nil, leaving the process in an unexpected directory.

Better approach: Capture the defer error and combine it with the function's return error:

var chdirErr error
defer func() {
    if err := os.Chdir(originalDir); err != nil {
        chdirErr = fmt.Errorf("failed to restore directory: %w", err)
        trialLog.Printf("failed to chdir back to %s: %v", originalDir, err)
    }
}()
// ...
if chdirErr != nil {
    return fmt.Errorf("operation succeeded but directory restore failed: %w", chdirErr)
}
return nil

if detectedHost != "github.com" {
addInteractiveLog.Printf("Auto-detected GHES host from git remote: %s", detectedHost)
os.Setenv("GH_HOST", detectedHost)
if err := os.Setenv("GH_HOST", detectedHost); err != nil {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] Missing test coverage for the failure scenario. This site differs from the others because it's not in a defer and the error is non-critical (feature detection fallback).

Test case needed:

func TestRunAddInteractive_FailedSetenvIsLogged(t *testing.T) {
    // Simulate os.Setenv failure by using an invalid env var name
    // (In practice, os.Setenv rarely fails on valid names, but the
    // code should handle it gracefully)
    
    // This test would verify that:
    // 1. The function continues execution despite Setenv failure
    // 2. The failure is logged (check log output)
    // 3. The subsequent workflow operations still succeed
}

Without this test, future refactors might accidentally change the continue-on-error behavior.

}
// Always restore to tempDir when this closure exits
defer os.Chdir(tempDir)
defer func() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnose] Nested defer in a closure creates a subtle bug risk. If the outer function panics after this closure completes, the inner defer (chdir back to tempDir) never runs.

Scenario:

  1. Closure starts in originalDir
  2. Closure defers chdir(tempDir)
  3. Closure completes, scheduling the defer
  4. Outer function panics before the deferred chdir executes
  5. Process is stuck in originalDir instead of tempDir

More robust: Avoid nested defers in closures. Either handle directory restoration at the outer function level, or ensure the closure defer executes immediately before returning.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 20, 2026

🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR correctly adds error handling for previously unchecked os.Setenv and os.Chdir calls, but the implementation has a critical issue in the secret-handling functions.

Critical Issue: Misleading Success After os.Setenv Failure

In engine_secrets.go, three functions (promptForCopilotPATUnified, promptForSystemTokenUnified, promptForGenericAPIKeyUnified) all follow the same problematic pattern:

  1. Attempt to store secret in environment with os.Setenv
  2. If it fails, log the error (debug-level only)
  3. Continue execution and print success message to user
  4. Return nil (success)

This means users see "✅ token received" even when the token was NOT stored in the environment, leading to confusing downstream failures.

The Good Parts

✅ All 6 os.Chdir error checks in defer blocks are well-implemented with proper logging and closure patterns.

Required Changes

Please update the three os.Setenv calls in engine_secrets.go to return errors instead of continuing after failure. The suggested fix is in the inline comments.

Note

🔒 Integrity filter blocked 1 item

The following item was blocked because it doesn't meet the GitHub integrity level.

  • #33465 pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".

To allow these resources, lower min-integrity in your GitHub frontmatter:

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

🔎 Code quality review by PR Code Quality Reviewer · ● 2.8M

Comment thread pkg/cli/engine_secrets.go
_ = os.Setenv(req.Name, token)
if err := os.Setenv(req.Name, token); err != nil {
engineSecretsLog.Printf("failed to set env var %s: %v", req.Name, err)
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Critical: When os.Setenv fails here, the code logs the error but continues to print the success message "Valid fine-grained Copilot token received" and returns nil. This is misleading - the token was received but NOT stored in the environment variable, which will cause downstream workflow failures.

Fix: Return an error after logging instead of continuing:

if err := os.Setenv(req.Name, token); err != nil {
    engineSecretsLog.Printf("failed to set env var %s: %v", req.Name, err)
    return fmt.Errorf("failed to store %s in environment: %w", req.Name, err)
}

The same issue exists in promptForSystemTokenUnified (line 379) and promptForGenericAPIKeyUnified (line 434).

os.Setenv("GH_HOST", detectedHost)
if err := os.Setenv("GH_HOST", detectedHost); err != nil {
addInteractiveLog.Printf("failed to set GH_HOST env var: %v", err)
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When os.Setenv("GH_HOST", detectedHost) fails, the error is only logged at debug-level, but the user still sees the info message "Auto-detected GitHub Enterprise host: X" (line 76). This creates a mismatch between what the user sees and the actual state.

Suggestion: Either return an error to fail fast, or at minimum print a warning to the user:

if err := os.Setenv("GH_HOST", detectedHost); err != nil {
    addInteractiveLog.Printf("failed to set GH_HOST env var: %v", err)
    fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Failed to set GH_HOST environment variable"))
    return fmt.Errorf("failed to set GH_HOST: %w", err)
}

@github-actions
Copy link
Copy Markdown
Contributor

🧪 Test Quality Sentinel Report

No test files were added or modified in this PR.

This PR fixes error handling for os.Setenv and os.Chdir calls across 10 sites in pkg/cli/, but does not include corresponding tests to verify the new error handling behavior.


Observation: Test Coverage Gap

The PR adds error checks and returns for:

  • os.Setenv failures (3 sites)
  • os.Chdir failures (7 sites)

While the Test Quality Sentinel only analyzes test quality (not test coverage), it's worth noting that none of these new error paths have test coverage.

Recommended follow-up (optional):

  • Add unit tests that verify the error handling behavior for os.Setenv and os.Chdir failures
  • Use table-driven tests with error injection to cover the failure cases
  • Example pattern:
    func TestFunctionWithSetenvError(t *testing.T) {
        // Test error path when os.Setenv fails
        // (May require refactoring to inject os operations for testability)
    }

Verdict: ✅ Test Quality Sentinel skipped (no tests to analyze).

🧪 Test quality analysis by Test Quality Sentinel · ● 2.1M ·

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Test Quality Sentinel: No test files were added or modified in this PR. Analysis skipped.

@pelikhan
Copy link
Copy Markdown
Collaborator

@copilot review all comments and pull request review comments

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skills-Based Review 🧠

Applied /diagnose and /tdd based on this being a bug fix for silent error handling in production code.

Key Themes

  1. Missing regression tests — The PR fixes 10 sites where errors were silently ignored, but adds no tests to verify the new error handling works or to prevent regression.

  2. Inconsistent error handling strategy — For os.Setenv (secrets), logging-only is dangerous: the code prints a success message even when the environment variable failed to set. Downstream code will fail mysteriously. For os.Chdir in defers, log-only is appropriate since these are cleanup operations.

  3. Test coverage gaps — The test files themselves still ignore os.Setenv/os.Chdir errors, demonstrating the same anti-pattern this PR fixes in production code.

Positive Highlights

  • Consistent fix pattern — All 10 sites use the same inline error handling approach
  • No new dependencies — Uses existing file-scoped loggers
  • Clear PR description — Impact and root cause are well-explained
  • Correct pattern for defer cleanup — The os.Chdir error handling in defers is appropriate

Verdict

Requesting changes for the os.Setenv error handling strategy. The current approach is unsafe for secret operations:

Problem: os.Setenv errors are logged but execution continues, printing a success message. The environment variable is not set, so downstream code will fail.

Solution: Fail fast when os.Setenv fails, or at minimum, skip the success message. See inline comments for specific suggestions.

The os.Chdir fixes in defer closures are fine as-is (log-only is appropriate for cleanup), but would benefit from regression tests.


Skills Applied:

  • /diagnose — Is the root cause addressed? Are there regression tests?
  • /tdd — Do tests cover the new error paths? Are edge cases tested?

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 5.3M

Comment thread pkg/cli/engine_secrets.go Outdated
// Store in environment for later use
_ = os.Setenv(req.Name, token)
if err := os.Setenv(req.Name, token); err != nil {
engineSecretsLog.Printf("failed to set env var %s: %v", req.Name, err)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] This error handling logs but doesn't fail. Consider whether silently continuing after a failed os.Setenv for a secret is safe:

  1. Line 332 prints "Valid fine-grained Copilot token received" (success message)
  2. But if os.Setenv failed, the environment variable is not set
  3. Downstream code expecting this env var will fail mysteriously

Suggestion: Either fail fast here, or at minimum, don't print the success message if os.Setenv fails:

if err := os.Setenv(req.Name, token); err != nil {
    return fmt.Errorf("failed to set env var %s: %w", req.Name, err)
}
fmt.Fprintln(os.Stderr, console.FormatSuccessMessage("Valid fine-grained Copilot token received"))

This applies to all 3 secret-handling sites in this file (lines 330, 380, 435).

return fmt.Errorf("failed to get current directory: %w", err)
}
defer os.Chdir(originalDir)
defer func() {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] The error handling pattern here is correct (log-only for cleanup operations in defer), but there's no test coverage for this error path.

Consider adding a test case that:

  1. Changes to a directory
  2. Deletes that directory
  3. Verifies the defer's os.Chdir error is handled gracefully

This would document the intended behavior when the working directory becomes invalid.

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented May 20, 2026

@copilot review all comments and pull request review comments

Addressed all review comments in the latest commit. Summary of changes:

  • engine_secrets.go (lines 329, 378, 433) — Replaced engineSecretsLog.Printf with return fmt.Errorf(...). The function now aborts and returns an error when os.Setenv fails, so the success message is suppressed and the caller sees the failure rather than a false positive.
  • add_interactive_orchestrator.go (line 73) — Added fmt.Fprintln(os.Stderr, console.FormatWarningMessage(...)) alongside the debug log, so a GH_HOST set failure is always visible.
  • deploy_command.go, update_command.go, trial_repository.go (3 sites), trial_helpers.go — Added fmt.Fprintln(os.Stderr, console.FormatWarningMessage(...)) in every os.Chdir restore defer, so working-directory restore failures are always visible in normal CLI runs (not gated by DEBUG).

Copilot AI requested a review from pelikhan May 20, 2026 06:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Silent error discards on os.Setenv / os.Chdir in pkg/cli (10 prod sites)

4 participants