Support temporary IDs for dispatched workflow runs#53572
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the implementation label and has only 20 new lines of code in business logic directories (threshold: 100).
|
|
✅ Ponytail Reviewer completed successfully! Ponytail review complete. Reviewed the PR diff (approve_workflow_run.cjs, dispatch_workflow.cjs, safe_outputs_dispatch.go, safe_outputs_validation_config.go, plus generated tool-schema/test files). The change reuses existing temporary_id.cjs helpers (resolveIssueNumber, loadTemporaryIdMapFromResolved) and existing FieldValidation config patterns rather than introducing new abstractions. It is a small, consistent wiring change with no speculative flexibility, dead code, or hand-rolled logic duplicating existing utilities. Lean already. Ship.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes
This patch introduces one real correctness hole in the new temporary-ID approval path, and it also recompiles existing lockfiles from strict to non-strict so policy-enabled installs would start failing those workflows at runtime.
Blocking themes
approve_workflow_runno longer consistently validates resolved temporary IDs as positive integer run IDs before calling the Actions API.- Two regenerated workflow lockfiles silently downgrade from strict compilation to non-strict, which is a behavior regression for environments enforcing
GH_AW_POLICY_STRICT=true.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 18.8 AIC · ⌖ 6.97 AIC · ⊞ 7K
Comment /review to run again
| const runId = parsePositiveInt(message.run_id); | ||
| return async function handleApproveWorkflowRun(message, resolvedTemporaryIds = {}) { | ||
| const resolvedRunId = resolveIssueNumber(message.run_id, loadTemporaryIdMapFromResolved(resolvedTemporaryIds)); | ||
| const runId = resolvedRunId.wasTemporaryId ? (resolvedRunId.resolved?.number ?? undefined) : parsePositiveInt(message.run_id); |
There was a problem hiding this comment.
The new approve_workflow_run path trusts resolvedTemporaryIds[...].number without re-validating it as a positive integer, so a malformed or fractional mapping can now flow straight into getWorkflowRun/approval instead of being rejected like direct run_id input is.
💡 Why this needs fixing
resolveIssueNumber() converts number with Number(...), but this call site never re-applies parsePositiveInt() when the source was a temporary ID:
const runId = resolvedRunId.wasTemporaryId
? (resolvedRunId.resolved?.number ?? undefined)
: parsePositiveInt(message.run_id);That means values like 1.5, 0, -1, or NaN coming from the resolved map can bypass the existing integer validation path and get sent to the Actions API. At best this becomes a flaky runtime failure; at worst it weakens the contract that approve_workflow_run only accepts positive integer run IDs.
Please normalize the resolved temporary-ID value through the same positive-integer validator before using it, and add a test that rejects a temporary ID mapped to a non-integer/non-positive number.
| @@ -153,7 +153,7 @@ jobs: | |||
| GH_AW_INFO_FIREWALL_TYPE: "squid" | |||
There was a problem hiding this comment.
These regenerated lockfiles now advertise GH_AW_COMPILED_STRICT: "false", so turning on GH_AW_POLICY_STRICT will hard-fail these workflows at runtime instead of preserving the previous strict behavior.
💡 Why this is a regression
This PR changes both metadata and runtime env from strict to non-strict and relies on the new enforcement step to catch it later:
GH_AW_COMPILED_STRICT: "false"
- name: Enforce strict mode policyThat is not a neutral refactor. Any repository/org that enables GH_AW_POLICY_STRICT=true will now see these workflows fail before doing useful work, even though the same lockfiles previously compiled as strict and ran successfully.
If the downgrade is intentional, the source workflow/frontmatter needs to be updated and justified. Otherwise this is a bad recompile and the lockfiles should stay strict so policy-enabled installations do not break.
There was a problem hiding this comment.
Review: Support temporary IDs for dispatched workflow runs
The implementation is well-structured and consistent across all layers (Go validation config, JSON schemas, CJS handlers, and tests).
Security: The cross-repo guard in approve_workflow_run.cjs correctly rejects temporary IDs whose repo doesn't match the current context.repo.owner/context.repo.repo, preventing a run from one repository being approved from another.
Correctness: The wasTemporaryId flag cleanly separates the two resolution paths — numeric IDs fall through to the existing parsePositiveInt path; temporary IDs are resolved via loadTemporaryIdMapFromResolved. The early !runId guard ensures the cross-repo check only fires when the ID was actually found in the resolved map.
Completeness: All schema files (schemas/agent-output.json, both copies of safe_outputs_tools.json), validation config (safe_outputs_validation_config.go), and the tool generator (safe_outputs_dispatch.go) are updated consistently. Tests cover the happy path, the cross-repo rejection, and the missing-run-ID warning.
No blocking issues found. ✅
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 35.9 AIC · ⌖ 8.83 AIC · ⊞ 5.7K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /grill-with-docs — requesting changes on three correctness/coverage gaps and one schema consistency issue.
📋 Key Themes & Highlights
Key Themes
- Undefined-repo guard (
approve_workflow_run.cjs): if a stored temporary-ID entry hasnumberbut norepo, the cross-repo comparison silently passes and the error message says "belongs toundefined". The guard needs to handle a missingrepofield explicitly, with a matching regression test. - Missing test for the no-run-id warning path (
dispatch_workflow.cjs): the branch that firescore.warningwhen the API returns noworkflow_run_idhas no automated test — thetemporaryIdfield absence is untested. - Schema gap (
schemas/agent-output.json):run_idinapprove_workflow_runaccepts any string; the temporary-IDpatternconstraint fromdispatch_workflow'stemporary_idshould be applied to the string branch here too. - Inputs-leak assertion missing (
safe_outputs_tools_loader.test.cjs): the new test confirmstemporary_idis passed to the handler, but doesn't assert it was removed frominputs.
Positive Highlights
- ✅ Cross-repo guard is a solid security boundary — good defensive design.
- ✅ Test coverage is thorough for the happy path and the cross-repo rejection case.
- ✅ Pattern validation is consistent across Go and JS validation layers.
- ✅ Clean separation:
temporary_iddestructuring insafe_outputs_tools_loader.cjskeeps the concern out of individual handlers.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 63.3 AIC · ⌖ 10.4 AIC · ⊞ 7.8K
Comment /matt to run again
| core.warning(error); | ||
| return { success: false, error }; | ||
| } | ||
| const resolvedRepo = resolvedRunId.resolved?.repo; |
There was a problem hiding this comment.
[/tdd] resolvedRepo can be undefined when a stored temporary ID entry has number but no repo field — the cross-repo guard would produce a misleading error like "belongs to undefined".
💡 Suggested fix + regression test
Guard before comparing:
const resolvedRepo = resolvedRunId.resolved?.repo;
if (resolvedRunId.wasTemporaryId) {
if (!resolvedRepo) {
const error = `Temporary workflow run ID '${message.run_id}' does not contain a repository reference`;
core.warning(error);
return { success: false, error };
}
if (resolvedRepo !== `${context.repo.owner}/${context.repo.repo}`) {
const error = `Temporary workflow run ID '${message.run_id}' belongs to ${resolvedRepo}, not ${context.repo.owner}/${context.repo.repo}`;
core.warning(error);
return { success: false, error };
}
}And add a test:
it("rejects a temporary ID with missing repo field", async () => {
const handler = await main(externalTokenConfig);
const result = await handler({ run_id: "aw_run123" }, { aw_run123: { number: 123 } });
expect(result.success).toBe(false);
expect(result.error).toContain("does not contain a repository reference");
});@copilot please address this.
| core.info(`✓ Successfully dispatched workflow: ${workflowFile} (run ID: ${runId})`); | ||
| } else { | ||
| core.info(`✓ Successfully dispatched workflow: ${workflowFile}`); | ||
| if (typeof message.temporary_id === "string") { |
There was a problem hiding this comment.
[/tdd] Missing test for dispatch_workflow when temporary_id is provided but the API returns no workflow_run_id — the warning branch fires but there's no assertion that temporaryId is absent from the response.
💡 Suggested test
it("should not include temporaryId when API returns no workflow_run_id", async () => {
github.rest.actions.createWorkflowDispatch.mockResolvedValueOnce({ data: {} }); // no workflow_run_id
const handler = await main({ workflows: ["test-workflow"], workflow_files: { "test-workflow": ".lock.yml" } });
const result = await handler({ type: "dispatch_workflow", workflow_name: "test-workflow", temporary_id: "aw_run123", inputs: {} }, {});
expect(result.success).toBe(true);
expect(result.temporaryId).toBeUndefined();
expect(core.warning).toHaveBeenCalledWith(expect.stringContaining("aw_run123"));
});Without this test the warning path is exercised only manually.
@copilot please address this.
| "run_id": { | ||
| "oneOf": [{ "type": "number" }, { "type": "string" }], | ||
| "description": "Positive integer workflow run ID to approve" | ||
| "description": "Positive integer workflow run ID to approve, or a temporary ID returned by dispatch_workflow" |
There was a problem hiding this comment.
[/grill-with-docs] The agent-output.json schema still uses "oneOf": [{ "type": "number" }, { "type": "string" }] for run_id in approve_workflow_run without adding a pattern constraint for the temporary ID string form — any arbitrary string passes JSON Schema validation.
💡 Suggested schema tightening
"run_id": {
"oneOf": [
{ "type": "number" },
{ "type": "string", "pattern": "^#?aw_[A-Za-z0-9_]{3,12}$" }
],
"description": "Positive integer workflow run ID to approve, or a temporary ID returned by dispatch_workflow"
}This keeps the schema consistent with the dispatch_workflow temporary_id pattern already defined in the same file.
@copilot please address this.
| const handlers = { defaultHandler: vi.fn(() => mockHandlerFunction) }; | ||
|
|
||
| const result = attachHandlers(tools, handlers); | ||
| result[0].handler({ environment: "staging", temporary_id: "aw_run123" }); |
There was a problem hiding this comment.
[/tdd] The safe_outputs_tools_loader.cjs test added for temporary_id passes the value through attachHandlers, but there is no test verifying that temporary_id is absent from the inputs object forwarded to the handler — confirming the destructuring split is correct.
💡 Suggested assertion to add
expect(mockHandlerFunction).toHaveBeenCalledWith(
expect.objectContaining({
temporary_id: "aw_run123",
inputs: expect.not.objectContaining({ temporary_id: expect.anything() }),
})
);Without this, a regression where temporary_id leaks into inputs would go undetected.
@copilot please address this.
There was a problem hiding this comment.
Pull request overview
Adds temporary-ID plumbing between workflow dispatch and run approval safe outputs.
Changes:
- Adds
temporary_idto dispatch schemas and generated tools. - Registers returned workflow run IDs and resolves them during approval.
- Updates tests and regenerated workflow artifacts.
Show a summary per file
| File | Description |
|---|---|
schemas/agent-output.json |
Extends dispatch and approval schemas. |
pkg/workflow/safe_outputs_validation_config.go |
Allows temporary IDs for approvals. |
pkg/workflow/safe_outputs_tools_generation_test.go |
Tests generated dispatch schemas. |
pkg/workflow/safe_outputs_dispatch.go |
Adds temporary IDs to dispatch tools. |
pkg/workflow/safe_output_validation_config_test.go |
Updates approval validation tests. |
pkg/workflow/js/safe_outputs_tools.json |
Updates embedded approval documentation. |
actions/setup/js/safe_outputs_tools.json |
Updates runtime tool documentation. |
actions/setup/js/safe_outputs_tools_loader.test.cjs |
Tests dispatch argument separation. |
actions/setup/js/safe_outputs_tools_loader.cjs |
Separates temporary IDs from workflow inputs. |
actions/setup/js/dispatch_workflow.test.cjs |
Tests returned run-ID mappings. |
actions/setup/js/dispatch_workflow.cjs |
Registers dispatched run mappings. |
actions/setup/js/approve_workflow_run.test.cjs |
Tests temporary-ID resolution and repository checks. |
actions/setup/js/approve_workflow_run.cjs |
Resolves temporary IDs before approval. |
.github/workflows/mcp-inspector.lock.yml |
Regenerates non-strict workflow metadata. |
.github/workflows/daily-team-evolution-insights.lock.yml |
Regenerates non-strict workflow metadata. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 15/15 changed files
- Comments generated: 3
- Review effort level: Balanced
| const resolvedRunId = resolveIssueNumber(message.run_id, loadTemporaryIdMapFromResolved(resolvedTemporaryIds)); | ||
| const runId = resolvedRunId.wasTemporaryId ? (resolvedRunId.resolved?.number ?? undefined) : parsePositiveInt(message.run_id); |
| DefaultMax: 1, | ||
| Fields: map[string]FieldValidation{ | ||
| "run_id": {Required: true, PositiveInteger: true}, | ||
| "run_id": {Required: true, IssueNumberOrTemporaryID: true}, |
| properties["temporary_id"] = map[string]any{ | ||
| "type": "string", | ||
| "description": "Optional temporary ID to associate with the dispatched workflow run. Use this ID as the run_id in a later approve_workflow_run call.", | ||
| "pattern": "^#?aw_[A-Za-z0-9_]{3,12}$", | ||
| } |
|
@copilot Please address the blocking review feedback below, refresh the branch if needed, then run the
|
|
👏 Great work! This PR looks ready for review. The temporary ID feature is well-implemented with:
The implementation correctly validates that temporary IDs are only resolved for runs in the current repository, which is a good security boundary. This follows the project agentic development process and maintains the quality standards of the gh-aw codebase.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot return workflow url if from another repo |
Agents can now assign a temporary ID when dispatching a workflow and reuse it to approve the returned workflow run.
Dispatch workflow
temporary_idto generated dispatch tools and output schemas.workflow_run_idin the safe-output temporary-ID map.Approve workflow run
run_id.{ "temporary_id": "aw_deploy1" }{ "run_id": "aw_deploy1" }Run: https://github.com/github/gh-aw/actions/runs/32103414328> Generated by 👨🍳 PR Sous Chef · gpt54 · 14 AIC · ⌖ 7.84 AIC · ⊞ 8.8K · ◷