fix: inline campaign discovery logic in campaign workflow step#20109
fix: inline campaign discovery logic in campaign workflow step#20109
Conversation
Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
|
@dsyme is this an outdated file? setup.sh might just be outdated too. |
|
@copilot investigate how this file is used |
Investigation complete. Here's how this file is used:
As for whether the |
|
Hey One thing is missing before this can be considered complete:
If you'd like a hand closing this gap, you can assign the following prompt to your coding agent:
|
|
@copilot This file is not needed in AW anymore. Remove it and integrate in the samples where needed |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in 2f80543. Removed |
There was a problem hiding this comment.
Pull request overview
This PR removes the standalone campaign_discovery.cjs module and inlines campaign discovery directly into the “Precompute campaign discovery” actions/github-script step for the security-alert-burndown campaign workflow.
Changes:
- Inline campaign discovery logic into
.github/workflows/security-alert-burndown.campaign.g.md(and recompiled lock workflow) instead of requiring/opt/gh-aw/actions/campaign_discovery.cjs. - Remove
actions/setup/js/campaign_discovery.cjsas an unused standalone module. - Update
actions/setup/README.mdto reflect the current.cjsfile count.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| actions/setup/js/campaign_discovery.cjs | Removed standalone campaign discovery module (logic now inlined in workflow step). |
| .github/workflows/security-alert-burndown.campaign.g.md | Inlines campaign discovery logic into the actions/github-script step and writes discovery results to /tmp/gh-aw/campaign-discovery.json. |
| .github/workflows/security-alert-burndown.campaign.g.lock.yml | Recompiled workflow reflecting the inlined discovery script. |
| actions/setup/README.md | Updates documented counts for copied .cjs files and total files. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| const campaignId = process.env.GH_AW_CAMPAIGN_ID; | ||
| const trackerLabel = process.env.GH_AW_TRACKER_LABEL; | ||
| const discoveryRepos = process.env.GH_AW_DISCOVERY_REPOS; | ||
| const cursorPath = process.env.GH_AW_CURSOR_PATH; | ||
| const maxItems = parseInt(process.env.GH_AW_MAX_DISCOVERY_ITEMS ?? "50", 10); |
There was a problem hiding this comment.
cursorPath is read from GH_AW_CURSOR_PATH but never used, and the discovery step does a full rescan every run. This conflicts with the workflow’s own instructions to use a durable cursor/checkpoint to avoid rescanning and to update the cursor file each run. Please either implement read/write of the cursor at cursorPath (e.g., boundary on updated_at + tie-breaker) or remove the cursor env var and the incremental-discovery requirements if they’re no longer applicable.
|
|
||
| for (const repoPath of repos) { | ||
| const parts = repoPath.split("/"); | ||
| if (parts.length !== 2) { core.warning(`Invalid repo format: "${repoPath}" — skipping`); continue; } | ||
| const [owner, repo] = parts; | ||
| let page = 1; | ||
| while (page <= maxPages && allItems.length < maxItems) { | ||
| const perPage = Math.min(30, maxItems - allItems.length); | ||
| try { | ||
| const response = await github.rest.issues.listForRepo({ owner, repo, labels: trackerLabel, state: "all", sort: "updated", direction: "desc", per_page: perPage, page }); |
There was a problem hiding this comment.
GH_AW_MAX_DISCOVERY_PAGES is enforced per-repo (while (page <= maxPages) inside the repo loop). If GH_AW_DISCOVERY_REPOS contains multiple repos, the step can exceed the intended per-run page budget (e.g., 3 pages * N repos). Consider tracking a global page counter across all repos (or rename the variable / document that the budget is per-repo) to ensure the configured pagination budget is actually respected.
| for (const repoPath of repos) { | |
| const parts = repoPath.split("/"); | |
| if (parts.length !== 2) { core.warning(`Invalid repo format: "${repoPath}" — skipping`); continue; } | |
| const [owner, repo] = parts; | |
| let page = 1; | |
| while (page <= maxPages && allItems.length < maxItems) { | |
| const perPage = Math.min(30, maxItems - allItems.length); | |
| try { | |
| const response = await github.rest.issues.listForRepo({ owner, repo, labels: trackerLabel, state: "all", sort: "updated", direction: "desc", per_page: perPage, page }); | |
| let pagesUsed = 0; | |
| for (const repoPath of repos) { | |
| const parts = repoPath.split("/"); | |
| if (parts.length !== 2) { core.warning(`Invalid repo format: "${repoPath}" — skipping`); continue; } | |
| const [owner, repo] = parts; | |
| let page = 1; | |
| while (allItems.length < maxItems && pagesUsed < maxPages) { | |
| const perPage = Math.min(30, maxItems - allItems.length); | |
| try { | |
| const response = await github.rest.issues.listForRepo({ owner, repo, labels: trackerLabel, state: "all", sort: "updated", direction: "desc", per_page: perPage, page }); | |
| pagesUsed++; |
| try { | ||
| fs.mkdirSync(GH_AW_TMP_DIR, { recursive: true }); | ||
| fs.writeFileSync(discoveryOutputPath, JSON.stringify(discoveryData, null, 2)); | ||
| } catch (err) { core.warning(`Failed to write discovery data: ${err.message}`); } |
There was a problem hiding this comment.
If writing campaign-discovery.json fails, the step only logs a warning but still sets discovery_file to the (potentially non-existent) path and continues as success. Since downstream logic expects this file to exist, this should fail the step (e.g., core.setFailed(...) / rethrow) when the directory creation or write fails, or otherwise guarantee the file exists before setting outputs.
| } catch (err) { core.warning(`Failed to write discovery data: ${err.message}`); } | |
| } catch (err) { | |
| core.setFailed(`Failed to write discovery data: ${err.message}`); | |
| return; | |
| } |
The compiled campaign workflow's "Precompute campaign discovery" step referenced
/opt/gh-aw/actions/campaign_discovery.cjs, which is not needed as a built-in AW module. Instead, the discovery logic is inlined directly into the step script in the campaign workflow.Changes
actions/setup/js/campaign_discovery.cjs(removed): Not needed as a standalone AW module.github/workflows/security-alert-burndown.campaign.g.md(updated): Discovery step script now inlines the logic directly usinggithub,core,fs, andpath(all natively available inactions/github-script), rather than delegating to an external.cjsmoduleGH_AW_TRACKER_LABELacrossGH_AW_DISCOVERY_REPOS, respectingGH_AW_MAX_DISCOVERY_ITEMS/GH_AW_MAX_DISCOVERY_PAGESbudgets/tmp/gh-aw/campaign-discovery.jsonfor the AI agent to read at runtimeitems_count,campaign_id,discovery_file.github/workflows/security-alert-burndown.campaign.g.lock.yml(recompiled): Reflects the inlined step scriptactions/setup/README.md(updated): Corrected.cjsfile count💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.