-
Notifications
You must be signed in to change notification settings - Fork 148
Use workflow-id instead of runId for stable island identification #15207
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.
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
Conversation
jobayertangail92-hub
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…identification Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot review work, add tests |
|
✅ Changeset Generator completed successfully! |
|
🧪 Smoke Project is now testing project operations... |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
Agent Container Tool Check
Result: 12/12 tools available ✅ PASS
|
|
✅ Smoke Project completed successfully. All project operations validated. |
Smoke Test Results - Run 21957895205✅ GitHub MCP Overall: PASS (8/9) cc @pelikhan
|
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
PR titles: Prevent workflow command injection in core.info() logging; Fix slide-deck-maintainer: require explicit safe output tool calls
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Updates replace-island body updates to use a stable workflow identifier instead of context.runId, so repeated workflow runs replace a single existing “island” section rather than appending new ones.
Changes:
- Switched island marker construction/searching from
runIdtoworkflowId - Updated action entrypoints to pass
GH_AW_WORKFLOW_IDintoupdateBody - Refreshed unit tests and fuzz harness inputs to use
workflowId
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/update_pr_description_helpers.cjs | Use workflowId for island marker generation and lookup in replace-island. |
| actions/setup/js/update_pr_description_helpers.test.cjs | Update helper tests to assert markers/search use workflowId. |
| actions/setup/js/update_issue.cjs | Pass workflowId (from GH_AW_WORKFLOW_ID) into updateBody. |
| actions/setup/js/update_issue.test.cjs | Update issue update tests to expect workflow-id based island markers. |
| actions/setup/js/update_pull_request.cjs | Pass workflowId (from GH_AW_WORKFLOW_ID) into updateBody. |
| actions/setup/js/update_release.cjs | Pass workflowId (from GH_AW_WORKFLOW_ID) into updateBody. |
| actions/setup/js/fuzz_update_body_harness.cjs | Switch fuzz harness params/input from runId to workflowId. |
| .changeset/patch-stable-island-markers.md | Add changeset entry documenting the patch behavior change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| */ | ||
| function updateBody(params) { | ||
| const { currentBody, newContent, operation, workflowName, runUrl, runId, includeFooter = true } = params; | ||
| const { currentBody, newContent, operation, workflowName, runUrl, workflowId, includeFooter = true } = params; |
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replace-island now fully depends on workflowId, but nothing here enforces that it’s non-empty. If callers pass \"\" (which they currently do as a default), all workflows without GH_AW_WORKFLOW_ID will share the same markers (<!-- gh-aw-island-start: -->), causing unrelated runs/workflows to overwrite each other’s islands. Consider failing fast for replace-island when workflowId is falsy (with an actionable message), or deriving a stable fallback identifier instead of allowing the empty string.
| @@ -85,24 +85,24 @@ function updateBody(params) { | |||
| } | |||
|
|
|||
| if (operation === "replace-island") { | |||
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replace-island now fully depends on workflowId, but nothing here enforces that it’s non-empty. If callers pass \"\" (which they currently do as a default), all workflows without GH_AW_WORKFLOW_ID will share the same markers (<!-- gh-aw-island-start: -->), causing unrelated runs/workflows to overwrite each other’s islands. Consider failing fast for replace-island when workflowId is falsy (with an actionable message), or deriving a stable fallback identifier instead of allowing the empty string.
| if (operation === "replace-island") { | |
| if (operation === "replace-island") { | |
| if (!workflowId) { | |
| throw new Error( | |
| "updateBody: 'workflowId' must be a non-empty string for 'replace-island' operation. " + | |
| "Set GH_AW_WORKFLOW_ID or pass a stable workflowId value." | |
| ); | |
| } |
| function buildIslandStartMarker(workflowId) { | ||
| return `<!-- gh-aw-island-start:${workflowId} -->`; |
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
workflowId is interpolated directly into an HTML comment marker. If it contains -->, newlines, or other unexpected characters, it can break marker structure and potentially enable marker spoofing/injection in the body. Recommend sanitizing/normalizing workflowId to a safe character set (and trimming) before building markers (e.g., restrict to [A-Za-z0-9._-] and replace others), and documenting the allowed format.
| function buildIslandEndMarker(workflowId) { | ||
| return `<!-- gh-aw-island-end:${workflowId} -->`; |
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
workflowId is interpolated directly into an HTML comment marker. If it contains -->, newlines, or other unexpected characters, it can break marker structure and potentially enable marker spoofing/injection in the body. Recommend sanitizing/normalizing workflowId to a safe character set (and trimming) before building markers (e.g., restrict to [A-Za-z0-9._-] and replace others), and documenting the allowed format.
| it("should build island start marker with workflow ID", () => { | ||
| const marker = buildIslandStartMarker("test-workflow"); | ||
| expect(marker).toBe("<!-- gh-aw-island-start:test-workflow -->"); |
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are good updates to move tests to workflowId, but the riskier new behaviors aren’t covered: (1) workflowId being empty/undefined (especially for replace-island) and (2) workflowId containing unsafe characters (e.g., whitespace, newlines, or -->). Add tests that assert the chosen behavior (throw, fallback, or sanitization) so marker stability/correctness doesn’t regress.
The
replace-islandoperation inupdateBodywas usingcontext.runIdfor island markers, causing each workflow run to create new islands instead of updating existing ones. The identifier must be stable across runs to enable content replacement.Changes
Island marker identification: Changed from numeric
runIdto stringworkflow-id(derived from workflow markdown filename)buildIslandStartMarker(workflowId)andbuildIslandEndMarker(workflowId)now use workflow-idfindIsland(body, workflowId)searches by stable identifierupdateBody()acceptsworkflowIdparameter instead ofrunIdCallers updated:
update_issue.cjs,update_pull_request.cjs,update_release.cjsnow passprocess.env.GH_AW_WORKFLOW_IDExample
Before (unstable):
After (stable):
Multiple runs now update the same island section instead of appending new ones.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.
Changeset