Skip to content

Eliminate duplicate constants: centralize DefaultLogDir, DefaultPayloadDir, DefaultPayloadSizeThreshold in config package#1620

Merged
lpcox merged 2 commits intomainfrom
copilot/fix-duplicate-code-issues-again
Mar 6, 2026
Merged

Eliminate duplicate constants: centralize DefaultLogDir, DefaultPayloadDir, DefaultPayloadSizeThreshold in config package#1620
lpcox merged 2 commits intomainfrom
copilot/fix-duplicate-code-issues-again

Conversation

Copy link
Contributor

Copilot AI commented Mar 6, 2026

internal/cmd package independently redeclared defaultPayloadDir, defaultPayloadSizeThreshold, and defaultLogDir — values already canonical in internal/config. This created silent drift risk: changes to config_payload.go would not propagate to the CLI layer.

Changes

  • internal/config/config_logging.go (new) — adds DefaultLogDir = "/tmp/gh-aw/mcp-logs" constant, mirroring the pattern already established by DefaultPayloadDir/DefaultPayloadSizeThreshold in config_payload.go
  • internal/cmd/flags_logging.go — removes the three duplicate local constants; references config.DefaultLogDir, config.DefaultPayloadDir, config.DefaultPayloadSizeThreshold
  • internal/cmd/root.go — updates two comparison guards that also relied on the removed local constants
  • internal/config/validation_env.go — replaces inline "/tmp/gh-aw/mcp-logs" literal with DefaultLogDir
  • internal/cmd/flags_logging_test.go — test expectations updated to reference config.Default* constants directly
// Before: duplicate definitions in two places
const defaultPayloadDir = "/tmp/jq-payloads"           // internal/cmd/flags_logging.go
const DefaultPayloadDir = "/tmp/jq-payloads"           // internal/config/config_payload.go

// After: single source of truth
config.DefaultPayloadDir  // referenced everywhere

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:

  • example.com
    • Triggering command: /tmp/go-build846392901/b301/launcher.test /tmp/go-build846392901/b301/launcher.test -test.testlogfile=/tmp/go-build846392901/b301/testlog.txt -test.paniconexit0 -test.timeout=10m0s conf�� 64/src/runtime/cgo credential.helper 64/bin/as (dns block)
  • invalid-host-that-does-not-exist-12345.com
    • Triggering command: /tmp/go-build846392901/b283/config.test /tmp/go-build846392901/b283/config.test -test.testlogfile=/tmp/go-build846392901/b283/testlog.txt -test.paniconexit0 -test.timeout=10m0s conf�� /go-build --local x_amd64/asm pull.rebase 64/src/runtime/c-unsafeptr=false (dns block)
  • nonexistent.local
    • Triggering command: /tmp/go-build846392901/b301/launcher.test /tmp/go-build846392901/b301/launcher.test -test.testlogfile=/tmp/go-build846392901/b301/testlog.txt -test.paniconexit0 -test.timeout=10m0s conf�� 64/src/runtime/cgo credential.helper 64/bin/as (dns block)
  • slow.example.com
    • Triggering command: /tmp/go-build846392901/b301/launcher.test /tmp/go-build846392901/b301/launcher.test -test.testlogfile=/tmp/go-build846392901/b301/testlog.txt -test.paniconexit0 -test.timeout=10m0s conf�� 64/src/runtime/cgo credential.helper 64/bin/as (dns block)
  • this-host-does-not-exist-12345.com
    • Triggering command: /tmp/go-build846392901/b310/mcp.test /tmp/go-build846392901/b310/mcp.test -test.testlogfile=/tmp/go-build846392901/b310/testlog.txt -test.paniconexit0 -test.timeout=10m0s conf�� --local user.name /opt/hostedtoolcache/go/1.25.7/x--property=MemoryHigh=170M 988407/b107/ (dns block)

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


🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

…in cmd package

Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Copilot AI changed the title [WIP] Analyze and resolve duplicate code issues in Go files Eliminate duplicate constants: centralize DefaultLogDir, DefaultPayloadDir, DefaultPayloadSizeThreshold in config package Mar 6, 2026
Copilot finished work on behalf of lpcox March 6, 2026 12:22
@lpcox lpcox marked this pull request as ready for review March 6, 2026 12:34
Copilot AI review requested due to automatic review settings March 6, 2026 12:34
@lpcox lpcox merged commit 341304b into main Mar 6, 2026
7 checks passed
@lpcox lpcox deleted the copilot/fix-duplicate-code-issues-again branch March 6, 2026 12:35
Copy link
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 centralizes CLI/logging default values by removing duplicated constants from internal/cmd and using the canonical defaults from internal/config, reducing the risk of silent drift between configuration and CLI behavior.

Changes:

  • Introduces config.DefaultLogDir alongside existing payload defaults in internal/config.
  • Updates CLI flag/env defaulting and guard comparisons to reference config.Default* constants.
  • Aligns tests and environment validation logic with the centralized defaults.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
internal/config/config_logging.go Adds DefaultLogDir constant as the canonical log directory default.
internal/cmd/flags_logging.go Removes local default constants and references config.Default* instead.
internal/cmd/root.go Updates comparisons that relied on removed local defaults to use config.Default*.
internal/config/validation_env.go Replaces inline default log-dir literal with DefaultLogDir.
internal/cmd/flags_logging_test.go Updates expectations to use config.Default* constants.

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

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.

3 participants