Refactor agent-session safe-output parsing to a single shared path#53840
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
PR Triage
Part of the same safe-output parsing refactor cluster. Recommend batch review with the other two PRs.
|
|
@copilot Remove create-agent-task add codemod to migrate |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Implemented in ec94ca5. |
|
Triage: category= Part of the B1-safeoutput-refactor batch (with #53841, #53838). Draft, no CI yet, no reviews. Undraft once ready and review together with the batch.
|
There was a problem hiding this comment.
Pull request overview
Centralizes agent-session configuration parsing, but unintentionally removes the promised deprecated create-agent-task compatibility path.
Changes:
- Extracts shared agent-session map parsing.
- Removes deprecated-key handling from parser, registry, and schema.
- Updates tests to expect deprecated keys to be ignored.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/create_agent_session.go |
Extracts parsing helper but removes fallback. |
pkg/workflow/create_agent_session_test.go |
Tests deprecated-key rejection. |
pkg/workflow/safe_output_handlers.go |
Removes deprecated handler alias. |
pkg/workflow/safe_outputs_fix_test.go |
Drops alias regression coverage. |
pkg/parser/schemas/main_workflow_schema.json |
Removes deprecated schema support. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 5
- Review effort level: Balanced
| @@ -34,7 +34,6 @@ var safeOutputHandlers = []safeOutputHandlerDescriptor{ | |||
| }, | |||
| { | |||
| Key: "create-agent-session", | |||
| if configData, exists := outputMap["create-agent-task"]; exists { | ||
| createAgentSessionLog.Print("WARNING: Using deprecated 'create-agent-task' configuration. Please migrate to 'create-agent-session' using 'gh aw fix'") | ||
| agentSessionConfig := &CreateAgentSessionConfig{} | ||
| return nil |
| if config != nil { | ||
| t.Fatal("parseAgentSessionConfig() should ignore deprecated create-agent-task key") |
| "safe-outputs": { | ||
| "type": "object", | ||
| "$comment": "Required if workflow creates or modifies GitHub resources. Operations requiring safe-outputs: approve-workflow-run, autofix-code-scanning-alert, add-comment, add-labels, add-reviewer, assign-milestone, assign-to-agent, assign-to-user, close-discussion, close-issue, close-pull-request, create-agent-session, create-agent-task (deprecated, use create-agent-session), create-check-run, create-code-scanning-alert, create-discussion, create-issue, create-project, create-project-status-update, create-pull-request, create-pull-request-review-comment, dispatch-workflow, hide-comment, link-sub-issue, mark-pull-request-as-ready-for-review, merge-pull-request, missing-data, missing-tool, noop, push-to-pull-request-branch, remove-labels, reply-to-pull-request-review-comment, resolve-pull-request-review-thread, set-issue-field, set-issue-type, submit-pull-request-review, threat-detection, unassign-from-user, update-discussion, update-issue, update-project, update-pull-request, update-release, upload-artifact, upload-asset. See documentation for complete details.", | ||
| "$comment": "Required if workflow creates or modifies GitHub resources. Operations requiring safe-outputs: approve-workflow-run, autofix-code-scanning-alert, add-comment, add-labels, add-reviewer, assign-milestone, assign-to-agent, assign-to-user, close-discussion, close-issue, close-pull-request, create-agent-session, create-check-run, create-code-scanning-alert, create-discussion, create-issue, create-project, create-project-status-update, create-pull-request, create-pull-request-review-comment, dispatch-workflow, hide-comment, link-sub-issue, mark-pull-request-as-ready-for-review, merge-pull-request, missing-data, missing-tool, noop, push-to-pull-request-branch, remove-labels, reply-to-pull-request-review-comment, resolve-pull-request-review-thread, set-issue-field, set-issue-type, submit-pull-request-review, threat-detection, unassign-from-user, update-discussion, update-issue, update-project, update-pull-request, update-release, upload-artifact, upload-asset. See documentation for complete details.", |
| @@ -44,7 +44,6 @@ func TestHasSafeOutputTypeNewKeys(t *testing.T) { | |||
| "upload-artifact", | |||
| "update-release", | |||
| "create-agent-session", | |||
|
@copilot remove all code for deprecated create-agent-task and add a codemod to migrate to create-agent-session |
|
✅ 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 #53840 does not have the 'implementation' label and has only 40 new lines of code in business logic directories (threshold: 100).
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
❌ Ponytail Reviewer failed. Please review the logs for details. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based 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.
Request changes
This patch is not a behavior-preserving refactor. It removes deprecated create-agent-task support from the parser entrypoint, the safe-output handler registry, and the JSON schema, while also adding a test that blesses the break.
Blocking themes
- legacy
create-agent-taskworkflows no longer parse into agent-session config - handler-table lookups stop recognizing the deprecated key, breaking registry-driven behavior
- schema validation now rejects the deprecated key before runtime parsing can recover
- the new regression test codifies the incompatible behavior instead of protecting parity
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 6.37 AIC · ⌖ 8.38 AIC · ⊞ 7K
Comment /review to run again
| @@ -34,7 +34,6 @@ var safeOutputHandlers = []safeOutputHandlerDescriptor{ | |||
| }, | |||
| { | |||
| Key: "create-agent-session", | |||
There was a problem hiding this comment.
Removing the deprecated alias here is not a harmless cleanup: registry-based callers like hasSafeOutputType, SafeOutputsConfigFromKeys, and any other key lookup through getSafeOutputHandlerByKey will stop recognizing create-agent-task, so existing workflows silently lose the handler before they ever reach your shared parser.
💡 Why this blocks merge
The PR description claims backward compatibility for the deprecated key, but this change severs that compatibility in the central dispatch table. That means legacy configs can now fail capability checks, permission derivation, or handler instantiation even if parsing code were kept compatible elsewhere.
Keep the alias until the deprecated key is intentionally removed everywhere, for example:
{
Key: "create-agent-session",
Aliases: []string{"create-agent-task"},
StructField: "CreateAgentSessions",
ToolName: "create_agent_session",
}Without that, this refactor changes behavior, not just structure.
| "safe-outputs": { | ||
| "type": "object", | ||
| "$comment": "Required if workflow creates or modifies GitHub resources. Operations requiring safe-outputs: approve-workflow-run, autofix-code-scanning-alert, add-comment, add-labels, add-reviewer, assign-milestone, assign-to-agent, assign-to-user, close-discussion, close-issue, close-pull-request, create-agent-session, create-agent-task (deprecated, use create-agent-session), create-check-run, create-code-scanning-alert, create-discussion, create-issue, create-project, create-project-status-update, create-pull-request, create-pull-request-review-comment, dispatch-workflow, hide-comment, link-sub-issue, mark-pull-request-as-ready-for-review, merge-pull-request, missing-data, missing-tool, noop, push-to-pull-request-branch, remove-labels, reply-to-pull-request-review-comment, resolve-pull-request-review-thread, set-issue-field, set-issue-type, submit-pull-request-review, threat-detection, unassign-from-user, update-discussion, update-issue, update-project, update-pull-request, update-release, upload-artifact, upload-asset. See documentation for complete details.", | ||
| "$comment": "Required if workflow creates or modifies GitHub resources. Operations requiring safe-outputs: approve-workflow-run, autofix-code-scanning-alert, add-comment, add-labels, add-reviewer, assign-milestone, assign-to-agent, assign-to-user, close-discussion, close-issue, close-pull-request, create-agent-session, create-check-run, create-code-scanning-alert, create-discussion, create-issue, create-project, create-project-status-update, create-pull-request, create-pull-request-review-comment, dispatch-workflow, hide-comment, link-sub-issue, mark-pull-request-as-ready-for-review, merge-pull-request, missing-data, missing-tool, noop, push-to-pull-request-branch, remove-labels, reply-to-pull-request-review-comment, resolve-pull-request-review-thread, set-issue-field, set-issue-type, submit-pull-request-review, threat-detection, unassign-from-user, update-discussion, update-issue, update-project, update-pull-request, update-release, upload-artifact, upload-asset. See documentation for complete details.", |
There was a problem hiding this comment.
Dropping create-agent-task from the schema makes legacy workflows fail validation before runtime parsing ever happens, so this is a hard compatibility break, not just dead-code cleanup.
💡 Why this blocks merge
safe-outputs is schema-validated with additionalProperties: false. Once this property disappears, any existing workflow still using the deprecated key is rejected immediately, which contradicts the PR description's claim that deprecated-key behavior is preserved.
If the intent is a real removal, it needs an explicit breaking-change path. If the intent is a refactor, keep the deprecated schema entry marked as deprecated until all downstream compatibility hooks are removed together.
| }) | ||
|
|
||
| if config != nil { | ||
| t.Fatal("parseAgentSessionConfig() should ignore deprecated create-agent-task key") |
There was a problem hiding this comment.
This test is locking in the regression instead of protecting compatibility: it asserts that the deprecated key should now be ignored, which is the opposite of the documented behavior and the PR description.
💡 Why this blocks merge
The old code intentionally supported create-agent-task with a warning, and the PR body explicitly says that behavior is preserved. A test that expects nil here will bless the breaking change and make future fixes look like regressions.
This should instead verify parity with create-agent-session, plus precedence when both keys are present, e.g. assert that base, target-repo, and default max still parse from the deprecated key.
| if configData, exists := outputMap["create-agent-task"]; exists { | ||
| createAgentSessionLog.Print("WARNING: Using deprecated 'create-agent-task' configuration. Please migrate to 'create-agent-session' using 'gh aw fix'") | ||
| agentSessionConfig := &CreateAgentSessionConfig{} | ||
| return nil |
There was a problem hiding this comment.
The deprecated-key fallback was removed from parseAgentSessionConfig, so a workflow that still uses safe-outputs.create-agent-task now gets nil instead of a parsed config. That is a breaking change disguised as a refactor.
💡 Why this blocks merge
Before this patch, create-agent-task still parsed with a warning; after this patch the function returns nil as soon as create-agent-session is absent. Any caller expecting deprecated-key compatibility now behaves as if agent-session output was never configured.
The extraction helper is fine, but the entrypoint still needs the old fallback:
if configData, exists := outputMap["create-agent-session"]; exists {
return c.parseAgentSessionConfigMap(configData)
}
if configData, exists := outputMap["create-agent-task"]; exists {
createAgentSessionLog.Print("WARNING: Using deprecated 'create-agent-task' configuration. Please migrate to 'create-agent-session' using 'gh aw fix'")
return c.parseAgentSessionConfigMap(configData)
}As written, this refactor breaks existing workflows and contradicts the stated preserved behavior.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /diagnosing-bugs — requesting changes for a backward-compatibility breakage that contradicts the PR's stated goals.
📋 Key Themes
Critical: Stated goal vs. actual behavior are inverted
The PR description says create-agent-task backward compat is preserved, but the diff breaks it in three coordinated places:
| Location | What was removed | Effect |
|---|---|---|
safe_output_handlers.go |
Aliases: []string{"create-agent-task"} |
hasSafeOutputType / SafeOutputsConfigFromKeys no longer resolve the deprecated key |
create_agent_session.go |
Entire create-agent-task fallback + deprecation warning |
Existing workflows using the old key silently produce no config |
main_workflow_schema.json |
Full create-agent-task property definition |
Legacy workflows fail schema validation before reaching the parser |
The new test (TestParseAgentSessionConfigWithDeprecatedKey) asserts config != nil is false when the deprecated key is used — encoding the breakage as expected behaviour rather than catching it as a regression.
What the fix looks like
Re-add the create-agent-task fallback in parseAgentSessionConfig (calling the new shared helper), restore the alias in safe_output_handlers.go, keep create-agent-task in the schema as deprecated, and flip the test assertion to verify the deprecated key does parse correctly with matching field values.
Positive Highlights
- ✅
parseAgentSessionConfigMapextraction is the right shape — a clean deep module - ✅ PR description and commit message are clear and well-structured
- ✅ New precedence test is the right kind of test — just needs the assertion corrected
The inline comments posted already cover each individual site. Addressing all five will bring the implementation in line with the PR's stated backward-compatibility promise.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 20.1 AIC · ⌖ 9.94 AIC · ⊞ 7.8K
Comment /matt to run again
There was a problem hiding this comment.
Review: Backward Compatibility Broken
This refactor removes create-agent-task support entirely rather than consolidating it onto the shared path. Three blocking issues:
-
safe_output_handlers.go— TheAliasesfield is removed, so registry functions (hasSafeOutputType,SafeOutputsConfigFromKeys) no longer resolvecreate-agent-task. Existing compiled workflows referencing this key will silently fail. -
create_agent_session.go— The deprecated-key fallback block (and its warning log) was deleted. Any workflow usingcreate-agent-task:now returnsnilconfig instead of being parsed and warned to migrate. -
main_workflow_schema.json— Removing thecreate-agent-taskschema property causes strict schema validation to reject legacy workflows before they reach the Go parser.
Suggested fix: route the deprecated key through parseAgentSessionConfigMap (call it for both keys in parseAgentSessionConfig), keep the schema entry marked deprecated: true, and restore the alias in the handler descriptor until a proper removal cycle is complete.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 19.6 AIC · ⌖ 8.88 AIC · ⊞ 5.7K
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot set create-agent-task as an x synonym in the json schema |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Removed runtime/schema support and stale documentation for |
|
@copilot review "gh agent-task" in docs and update with latest gh command name (checks github docs) |
|
@pelikhan Unfortunately I hit an unexpected error while processing your comment. I've automatically reported this to GitHub. You can ask me to try again later by mentioning me in a new comment. If you want to contact GitHub about this error, please mention the following identifier so they can better serve you: Sorry for the inconvenience! |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
parseAgentSessionConfighad two near-identical branches forcreate-agent-sessionand deprecatedcreate-agent-task, creating drift risk for future field changes. This PR centralizes parsing while preserving key-precedence and deprecation behavior.What changed
parseAgentSessionConfigMap(configData any).parseAgentSessionConfig:create-agent-sessioncreate-agent-taskwith existing warningBehavior preserved
base,target-repo, and base safe-output fields are parsed identically for both keys.target-repostill returnsnil(validation path unchanged).maxto1.Coverage updates
create-agent-sessionwins when both keys are present.