Uh oh!
There was an error while loading. Please reload this page.
fix(safe-outputs): re-derive patch/bundle paths and stop double-writing infrastructure fields - #37299
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Fixes safe-output handler failures where create_pull_request / push_to_pull_request_branch could not locate patch/bundle files after validateItem() stripped transport fields during NDJSON normalization, by re-deriving expected on-disk paths from message.branch (+ optional repo context).
Changes:
- Added
resolve_transport_paths.cjshelper to re-derivepatch_path/bundle_pathfrombranch(+repo/defaultTargetRepo) by checking existing candidate paths. - Wired the resolver into
create_pull_request.cjsandpush_to_pull_request_branch.cjsso handlers can recover when paths were stripped. - Added a dedicated Vitest suite covering passthrough, recovery, fallback ordering, and missing-file cases.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/resolve_transport_paths.cjs | New helper to recover patch/bundle transport paths from branch/repo context. |
| actions/setup/js/resolve_transport_paths.test.cjs | New unit tests validating recovery and fallback behavior. |
| actions/setup/js/create_pull_request.cjs | Uses resolver instead of relying on stripped patch_path/bundle_path. |
| actions/setup/js/push_to_pull_request_branch.cjs | Uses resolver instead of relying on stripped patch_path/bundle_path. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 4
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
3eae657 to
6b951d4Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Fix failing test/build/lint jobs and rerun checks.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Uh oh!
There was an error while loading. Please reload this page.
✅ smoke-ci: safeoutputs CLI comment + comment-memory run (27062132969)
|
fix(safe-outputs): re-derive patch/bundle paths and stop double-writing infrastructure fields
Summary
This PR delivers two orthogonal but related sets of changes:
Security / correctness fix in safe-outputs —
patch_pathandbundle_pathare no longer written onto safe-output entries by the MCP server. Instead, they are re-derived on the privileged host side from the validatedbranch(and optionalrepo) fields. A newresolve_transport_paths.cjsmodule encapsulates the derivation logic. The now-obsolete "infrastructure field stripping" block in the type validator is removed because agents can no longer inject arbitrary file paths via those fields.Terminology rename — Every reference to
max-effective-tokens/GH_AW_DEFAULT_MAX_EFFECTIVE_TOKENS/ Effective Tokens (ET) across docs, specs, workflow frontmatter, golden files, error templates, and compiled Go code is replaced withmax-ai-credits/GH_AW_DEFAULT_MAX_AI_CREDITS/ AI Credits (AIC). The old backward-compatibility note is also removed since the deprecation period is over.Changes by area
actions/setup/js/— safe-outputs host-side infrastructure (root fix)safe_outputs_handlers.cjsentry.patch_path = ...andentry.bundle_path = ...assignments from both thecreate_pull_requestandpush_to_pull_request_branchhandlers. Paths are now re-derived frombranchand never transmitted through the entry.safe_output_type_validator.cjspatch_path/bundle_pathare no longer present on entries;base_commitanddiff_sizeare legitimate metadata and should pass through.safe_output_type_validator.test.cjsbase_commit/diff_sizeare asserted present onnormalizedItem.resolve_transport_paths.cjsbranchfield. Iterates repo candidates (message.repo→defaultTargetRepo→null) and checksfs.existsSync()for each derived path. ExportsresolveTransportPaths(message, defaultTargetRepo).resolve_transport_paths.test.cjsdefaultTargetRepofallback, single-repo fallback when multi-repo misses, no-file case, bundle-only case.create_pull_request.cjspullRequestItem.patch_path/bundle_pathwithresolveTransportPaths(pullRequestItem, defaultTargetRepo); added null-guardif (!isEmpty && patchFilePath).create_pull_request.test.cjscanonicalPatchPath(),canonicalBundlePath(),cleanupCanonicalTransports()helpers +beforeEach/afterEachhooks; all handler call-sites now pass{ branch: "..." }instead of{ patch_path: ... }.push_to_pull_request_branch.cjscreate_pull_request.cjs— replaced direct path reads withresolveTransportPaths.push_to_pull_request_branch.test.cjscreate_pull_request.test.cjs;createPatchFile()gainsbranchas its first argument.collect_ndjson_output.cjsactions/setup/js/— path-derivation function renames (breaking: internal exports)getPatchPathgetPatchPathForBranchgit_patch_utils.cjsgetPatchPathForRepogetPatchPathForBranchInRepogit_patch_utils.cjsgetBundlePathgetBundlePathForBranchgenerate_git_bundle.cjsgetBundlePathForRepogetBundlePathForBranchInRepogenerate_git_bundle.cjsAll corresponding test files (
git_patch_utils.test.cjs,generate_git_patch.test.cjs,find_repo_checkout.test.cjs) and import sites updated accordingly.Terminology rename: Effective Tokens → AI Credits
All of the following are mechanical search-and-replace across docs, specs, workflows, and source:
max-effective-tokens:(frontmatter key)max-ai-credits:GH_AW_DEFAULT_MAX_EFFECTIVE_TOKENS(env var)GH_AW_DEFAULT_MAX_AI_CREDITStotal_effective_tokens(metric field)default_max_effective_tokensdefault_max_ai_creditsGetMaxEffectiveTokens()(Go method)GetMaxAICredits()remains)Files touched (terminology):
.changeset/patch-bump-awf-v0-25-44.md.github/aw/syntax-agentic.md,token-optimization.md.github/workflows/— 6 workflow frontmatter filesDICTATION.mdactions/setup/js/handle_agent_failure.test.cjs(rate-limit error message)actions/setup/md/effective_tokens_rate_limit_error.mddocs/adr/35286-compiler-managed-enterprise-env-controls.mddocs/src/content/docs/— 10 reference/guide/experimental docspkg/workflow/engine.goscratchpad/token-budget-guidelines.mdspecs/aw-harness.md(breaking: spec key changed)Golden files —
GH_AW_TIMEOUT_MINUTESenv varAdded
GH_AW_TIMEOUT_MINUTESto all affected WASM golden snapshots:TestWasmGolden_AllEngines/copilot.goldenGH_AW_TIMEOUT_MINUTESTestWasmGolden_CompileFixtures/basic-copilot.golden10TestWasmGolden_CompileFixtures/playwright-cli-mode.golden20TestWasmGolden_CompileFixtures/smoke-copilot.golden15TestWasmGolden_CompileFixtures/with-imports.golden10Action pins
pkg/actionpins/data/action_pins.jsonandpkg/workflow/data/action_pins.jsonupdated (version bumps from workflow recompiles).Why re-derive instead of strip?
Previously, the MCP server wrote
patch_path/bundle_pathonto each safe-output entry, and the type validator stripped those fields as a security measure to prevent agents from injecting arbitrary host paths. This created a redundant write-then-strip cycle.The new approach: the MCP server never writes these paths onto entries (so there is nothing to inject), and the host-side action re-derives the correct paths from the already-validated
branchfield usingresolveTransportPaths. This eliminates the attack surface at the source rather than patching it at the sink.Breaking changes
getPatchPath,getPatchPathForRepo,getBundlePath,getBundlePathForReporenamedmax-effective-tokens:→max-ai-credits:inspecs/aw-harness.mdand all workflow frontmatterGH_AW_DEFAULT_MAX_EFFECTIVE_TOKENS→GH_AW_DEFAULT_MAX_AI_CREDITSEngineConfig.GetMaxEffectiveTokens()removed