Skip to content

Refactor token env lookup and schedule helpers to remove semantic drift #45298

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.

Sign up for GitHub

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

Merged
pelikhan merged 3 commits into from
Jul 14, 2026
Merged

Refactor token env lookup and schedule helpers to remove semantic drift #45298

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b222670
Initial plan
Copilot Jul 13, 2026
76e94e3
chore: outline plan for semantic clustering refactor
Copilot Jul 13, 2026
344d3b1
refactor parser and schedule helpers to reduce semantic drift
Copilot Jul 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions .github/workflows/smoke-call-workflow.lock.yml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 1 addition & 5 deletions pkg/parser/github.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
"strings"

"github.com/github/gh-aw/pkg/constants"
"github.com/github/gh-aw/pkg/envutil"
"github.com/github/gh-aw/pkg/logger"
"github.com/github/gh-aw/pkg/stringutil"
)
Expand Down Expand Up @@ -63,10 +62,7 @@ func GetGitHubToken() (string, error) {
githubLog.Print("Getting GitHub token")

// First try environment variable
if token := envutil.GetStringFromEnv("GITHUB_TOKEN", "", githubLog); token != "" {
return token, nil
}
if token := envutil.GetStringFromEnv("GH_TOKEN", "", githubLog); token != "" {
if token := githubTokenFromEnv(githubLog); token != "" {
return token, nil
}

Expand Down
13 changes: 13 additions & 0 deletions pkg/parser/github_token_env.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package parser

import (
"github.com/github/gh-aw/pkg/envutil"
"github.com/github/gh-aw/pkg/logger"
)

func githubTokenFromEnv(log *logger.Logger) string {
if token := envutil.GetStringFromEnv("GITHUB_TOKEN", "", log); token != "" {
return token
}
return envutil.GetStringFromEnv("GH_TOKEN", "", log)
}
33 changes: 33 additions & 0 deletions pkg/parser/github_token_env_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
//go:build !integration

package parser

import (
"testing"

"github.com/github/gh-aw/pkg/logger"
"github.com/stretchr/testify/assert"
)

func TestGithubTokenFromEnv(t *testing.T) {
t.Run("prefers GITHUB_TOKEN over GH_TOKEN", func(t *testing.T) {
t.Setenv("GITHUB_TOKEN", "github-token")
t.Setenv("GH_TOKEN", "gh-token")

assert.Equal(t, "github-token", githubTokenFromEnv(logger.New("parser:test")))
})

t.Run("falls back to GH_TOKEN when GITHUB_TOKEN is empty", func(t *testing.T) {
t.Setenv("GITHUB_TOKEN", "")
t.Setenv("GH_TOKEN", "gh-token")

@github-actions github-actions Bot Jul 14, 2026

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 second and third test cases pass nil as the logger while the first passes logger.New("parser:test"). There is no documented reason for the inconsistency — if nil is intentionally valid (i.e. the helper is safe to call without a logger), make that explicit with a comment or a dedicated TestGithubTokenFromEnvNilLogger case; otherwise use logger.New(...) consistently to avoid masking nil-pointer panics during future edits.

@copilot please address this.


assert.Equal(t, "gh-token", githubTokenFromEnv(nil))
})

t.Run("returns empty when neither token is set", func(t *testing.T) {
t.Setenv("GITHUB_TOKEN", "")
t.Setenv("GH_TOKEN", "")

assert.Empty(t, githubTokenFromEnv(nil))
})
}
7 changes: 1 addition & 6 deletions pkg/parser/github_wasm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,12 @@ package parser

import (
"errors"

"github.com/github/gh-aw/pkg/envutil"
)

func GetGitHubToken() (string, error) {
// Wasm callers do not use the package logger, so pass nil to suppress debug
// logging while still centralizing environment variable access.
if token := envutil.GetStringFromEnv("GITHUB_TOKEN", "", nil); token != "" {
return token, nil
}
if token := envutil.GetStringFromEnv("GH_TOKEN", "", nil); token != "" {
if token := githubTokenFromEnv(nil); token != "" {
return token, nil
}
return "", errors.New("GitHub token not available in Wasm (set GITHUB_TOKEN or GH_TOKEN environment variable)")
Expand Down
60 changes: 29 additions & 31 deletions pkg/parser/schedule_cron_detection.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,28 @@ var cronDetectionLog = logger.New("parser:schedule_cron_detection")
// cronFieldPattern matches valid cron field syntax (pre-compiled for performance)
var cronFieldPattern = regexp.MustCompile(`^[\d\*\-/,]+$`)

func cronFields(cron string) ([]string, bool) {
fields := strings.Fields(cron)
return fields, len(fields) == 5
}

func isNumericCronField(field string) bool {
if field == "" {
return false
}
for _, ch := range field {
if ch < '0' || ch > '9' {
return false
}
}
return true
}

// IsDailyCron checks if a cron expression represents a daily schedule at a fixed time
// (e.g., "0 0 * * *", "30 14 * * *", etc.)
func IsDailyCron(cron string) bool {
fields := strings.Fields(cron)
if len(fields) != 5 {
fields, ok := cronFields(cron)
if !ok {
return false
}
// Daily pattern: minute hour * * *
Expand All @@ -32,16 +49,8 @@ func IsDailyCron(cron string) bool {
minute := fields[0]
hour := fields[1]

// Minute and hour should be digits only (no *, /, -, ,)
for _, ch := range minute {
if ch < '0' || ch > '9' {
return false
}
}
for _, ch := range hour {
if ch < '0' || ch > '9' {
return false
}
if !isNumericCronField(minute) || !isNumericCronField(hour) {
return false
}

result := fields[2] == "*" && fields[3] == "*" && fields[4] == "*"
Expand All @@ -54,8 +63,8 @@ func IsDailyCron(cron string) bool {
// IsHourlyCron checks if a cron expression represents an hourly interval with a fixed minute
// (e.g., "0 */1 * * *", "30 */2 * * *", etc.)
func IsHourlyCron(cron string) bool {
fields := strings.Fields(cron)
if len(fields) != 5 {
fields, ok := cronFields(cron)
if !ok {
return false
}
// Hourly pattern: minute */N * * * or minute *N * * *
Expand All @@ -65,11 +74,8 @@ func IsHourlyCron(cron string) bool {
minute := fields[0]
hour := fields[1]

// Minute should be digits only (no *, /, -, ,)
for _, ch := range minute {
if ch < '0' || ch > '9' {
return false
}
if !isNumericCronField(minute) {
return false
}

// Hour should be an interval pattern like */N
Expand All @@ -88,8 +94,8 @@ func IsHourlyCron(cron string) bool {
// IsWeeklyCron checks if a cron expression represents a weekly schedule at a fixed time
// (e.g., "0 0 * * 1", "30 14 * * 5", etc.)
func IsWeeklyCron(cron string) bool {
fields := strings.Fields(cron)
if len(fields) != 5 {
fields, ok := cronFields(cron)
if !ok {
return false
}
// Weekly pattern: minute hour * * DOW
Expand All @@ -101,16 +107,8 @@ func IsWeeklyCron(cron string) bool {
minute := fields[0]
hour := fields[1]

// Minute and hour should be digits only (no *, /, -, ,)
for _, ch := range minute {
if ch < '0' || ch > '9' {
return false
}
}
for _, ch := range hour {
if ch < '0' || ch > '9' {
return false
}
if !isNumericCronField(minute) || !isNumericCronField(hour) {
return false
}

// Check day-of-month and month are wildcards
Expand Down
23 changes: 22 additions & 1 deletion pkg/parser/schedule_cron_detection_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ func TestIsCronExpression(t *testing.T) {
{"invalid expression", "invalid cron expression", false},
{"empty string", "", false},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
result := IsCronExpression(tt.input)
Expand All @@ -37,6 +36,28 @@ func TestIsCronExpression(t *testing.T) {
}
}

func TestIsNumericCronField(t *testing.T) {
tests := []struct {
name string
input string
expected bool
}{
{"single digit", "5", true},
{"multiple digits", "15", true},
{"empty", "", false},
{"wildcard", "*", false},
{"interval", "*/5", false},
{"range", "1-5", false},
{"list", "1,2", false},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assert.Equal(t, tt.expected, isNumericCronField(tt.input))
})
}
}

func TestIsDailyCron(t *testing.T) {
tests := []struct {
name string

@github-actions github-actions Bot Jul 14, 2026

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] cronFields is extracted as a standalone helper but has no direct test — it is only exercised indirectly through IsDailyCron/IsHourlyCron/IsWeeklyCron. A direct test would lock in its contract (5-field split, short-circuit on wrong field count) and prevent silent regressions if the helper is later reused.

💡 Suggested test skeleton 
func TestCronFields(t *testing.T) {
    fields, ok := cronFields("0 0 * * *")
    assert.True(t, ok)
    assert.Equal(t, []string{"0", "0", "*", "*", "*"}, fields)

    _, ok = cronFields("0 0 * *") // only 4 fields
    assert.False(t, ok)

    _, ok = cronFields("") // empty string
    assert.False(t, ok)
}

@copilot please address this.

Expand Down
28 changes: 12 additions & 16 deletions pkg/workflow/schedule_preprocessing.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,12 +472,7 @@ func (c *Compiler) addDailyCronWarning(cronExpr string) {
hour, minute,
)

// This warning is added to the warning count
// It will be collected and displayed by the compilation process
c.IncrementWarningCount()

// Store the warning for later display
c.addScheduleWarning(warningMsg)
c.emitScheduleWarning(warningMsg)
}
}

Expand All @@ -499,11 +494,7 @@ func (c *Compiler) addHourlyCronWarning(cronExpr string) {
minute, interval,
)

// This warning is added to the warning count
c.IncrementWarningCount()

// Store the warning for later display
c.addScheduleWarning(warningMsg)
c.emitScheduleWarning(warningMsg)
}
}

Expand Down Expand Up @@ -538,14 +529,19 @@ func (c *Compiler) addWeeklyCronWarning(cronExpr string) {
weekdayName, hour, minute, strings.ToLower(weekdayName),
)

// This warning is added to the warning count
c.IncrementWarningCount()

// Store the warning for later display
c.addScheduleWarning(warningMsg)
c.emitScheduleWarning(warningMsg)
}
}

func (c *Compiler) emitScheduleWarning(warning string) {
// This warning is added to the warning count.
// It will be collected and displayed by the compilation process.
c.IncrementWarningCount()

// Store the warning for later display.
c.addScheduleWarning(warning)
}

// addScheduleWarning adds a warning to the compiler's schedule warnings list
func (c *Compiler) addScheduleWarning(warning string) {
if c.scheduleWarnings == nil {
Expand Down
Loading