Refactor token env lookup and schedule helpers to remove semantic drift#45298
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Hey This PR is in early WIP stages with no diff yet. Once the implementation lands, here are a few things to verify:
If you would like a hand, you can assign this prompt to your coding agent:
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
PR Triage (run 29275650494)\n\nCategory: refactor | Risk: medium | Score: 55/100 | Action: batch_review | Batch: draft-quality\n\nScore: Impact 25/50 + Urgency 15/30 + Quality 15/20\n\nCentralizes token env lookup and schedule helpers. 8 files, +129/-62. DRAFT. Similar in nature to #45280 and #45282 - good candidate for batch review.
|
There was a problem hiding this comment.
Pull request overview
Centralizes token lookup, cron parsing, and schedule-warning bookkeeping to reduce duplicated logic.
Changes:
- Shares GitHub token environment precedence across build targets.
- Extracts common cron and warning helpers with regression tests.
- Refreshes the smoke workflow lockfile.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/schedule_preprocessing.go |
Centralizes warning emission. |
pkg/parser/schedule_cron_detection.go |
Adds shared cron helpers. |
pkg/parser/schedule_cron_detection_test.go |
Tests numeric-field detection. |
pkg/parser/github.go |
Uses shared token lookup. |
pkg/parser/github_wasm.go |
Uses shared token lookup for Wasm. |
pkg/parser/github_token_env.go |
Implements token precedence helper. |
pkg/parser/github_token_env_test.go |
Tests token precedence and fallback. |
.github/workflows/smoke-call-workflow.lock.yml |
Refreshes generated inputs, permissions, and secrets. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Medium
| actions: read | ||
| contents: read | ||
| pull-requests: read | ||
| issues: write | ||
| pull-requests: write |
There was a problem hiding this comment.
I re-ran the branch validations (make fmt, make lint, make test-unit, make test, make recompile, make agent-report-progress).
I also re-tested a direct lockfile revert for this file; it still fails check-workflow-drift because the current workflow source recompiles to this lockfile shape. So I did not push a revert that would leave the repo in a drifted state.
|
Run: https://github.com/github/gh-aw/actions/runs/29286484887
|
Completed a PR-finisher pass on #45298.
No new commit was made in this pass. CI on this head still requires a human re-trigger if a fresh run is needed. |
|
Run: https://github.com/github/gh-aw/actions/runs/29289965782
|
Done. I ran
All passed. No new commit was made in this pass. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, focused refactoring with no blocking issues.\n\n- Token env lookup correctly preserves GITHUB_TOKEN → GH_TOKEN precedence across both wasm and non-wasm paths.\n- isNumericCronField and cronFields helpers are correct and well-tested.\n- emitScheduleWarning correctly atomizes the increment+store pairing.\n- Lock file change (replacing secrets: inherit with explicit list) is a security improvement.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 12.4 AIC · ⌖ 6.7 AIC · ⊞ 4.8K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — two minor observations, no blocking issues.
📋 Key Themes & Highlights
Key Themes
cronFieldslacks a direct unit test — only covered indirectly; a dedicated test would lock in its contract.- Inconsistent logger usage in
github_token_env_test.go—nilvslogger.New(...)across subtests without explanation.
Positive Highlights
- ✅ Clean, minimal helpers with well-scoped responsibilities (
githubTokenFromEnv,isNumericCronField,emitScheduleWarning). - ✅ Wasm and non-wasm paths now share a single precedence rule — drift eliminated.
- ✅
TestIsNumericCronFieldtable covers the important edge cases (wildcards, intervals, ranges). - ✅
emitScheduleWarningremoves the boilerplate comments that were being copied three times — good signal cleanup.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 31 AIC · ⌖ 4.54 AIC · ⊞ 6.6K
Comment /matt to run again
|
|
||
| func TestIsDailyCron(t *testing.T) { | ||
| tests := []struct { | ||
| name string |
There was a problem hiding this comment.
[/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.
|
|
||
| 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") |
There was a problem hiding this comment.
[/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.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (7 tests)
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (111 new lines across
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Review: medium-risk maintenance issues only
The Go refactors (token env helper, cron field helpers, emitScheduleWarning) are mechanically correct and have adequate test coverage. Two medium-risk issues flagged above.
Findings summary
Medium — emitScheduleWarning / addScheduleWarning split (schedule_preprocessing.go:349): The lower-level addScheduleWarning method (which skips the counter) remains package-accessible. A future caller adding a new warning type via that method directly will silently miscount warnings. Rename it to make the bypass-unsafe path obvious.
Medium — secrets: inherit → explicit list in lock file (smoke-call-workflow.lock.yml:37): Static list is a maintenance snapshot; future secret additions will silently not be forwarded. If this is intentional (scope-limiting), document it clearly.
The permission escalation (pull-requests: write, issues: write) was already flagged in a prior review and is outside this pass's scope.
🔎 Code quality review by PR Code Quality Reviewer · 40.7 AIC · ⌖ 4.57 AIC · ⊞ 5.4K
Comment /review to run again
Comments that could not be inline-anchored
pkg/workflow/schedule_preprocessing.go:349
Silent warning-count miss if addScheduleWarning is called directly: emitScheduleWarning is the correct entry point, but addScheduleWarning (which skips IncrementWarningCount) remains callable from within the same package.
<details>
<summary>💡 Suggested fix</summary>
Rename addScheduleWarning to storeScheduleWarning (or similar) and update emitScheduleWarning to call the renamed method. Keeping the raw storage method named addScheduleWarning invites future callers to accid…
.github/workflows/smoke-call-workflow.lock.yml:37
Switching from secrets: inherit to an explicit list silently drops any future secrets: if the called workflow is updated to require new secrets, callers receive empty values with no error — only silent downstream auth failures.
<details>
<summary>💡 Details</summary>
secrets: inherit forwards all repository secrets automatically. The explicit list here is a static snapshot. As the called workflow evolves, each new secret must also be manually added to this caller — and since this is a…
This addresses semantic clustering drift in three hotspots: duplicated GitHub token env lookup across build variants, repeated numeric cron-field parsing, and repeated schedule warning emission boilerplate. The refactor centralizes these behaviors without changing user-facing semantics.
Token lookup precedence unified across wasm/non-wasm
githubTokenFromEnv(log *logger.Logger) stringinpkg/parser/github_token_env.go.GetGitHubTokenvariants to use the sameGITHUB_TOKEN→GH_TOKENlookup path.gh auth tokenfallback behavior unchanged.Cron numeric-field logic consolidated
isNumericCronField(string) booland sharedcronFields(...)([]string, bool)inpkg/parser/schedule_cron_detection.go.IsDailyCron,IsHourlyCron, andIsWeeklyCronto use shared helpers instead of inline repeated scans.Schedule warning bookkeeping centralized
emitScheduleWarning(warning string)inpkg/workflow/schedule_preprocessing.go.Focused regression coverage for extracted helpers
pkg/parser/github_token_env_test.go(env precedence/fallback cases).pkg/parser/schedule_cron_detection_test.gowithTestIsNumericCronField.