Repository navigation
Harden Pi provider retries and failure diagnostics - #65562
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
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. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. 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.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
It rejects compiler-generated queue expressions, removes active command routes, and leaks secrets through agent_end snapshots.
Review effort: Balanced
Findings: 3
Open (3)
What changed in this PR
Hardens Pi provider execution with retries and sanitized failure diagnostics.
Changes:
- Configures bounded Pi SDK retries.
- Adds provider error extraction, redaction, reporting, and tests.
- Regenerates workflow routing and adjusts concurrency schema validation.
| File | Description |
|---|---|
pkg/workflow/schemas/github-workflow.json |
Restricts concurrency queue values. |
go.mod |
Promotes YAML v3 to a direct dependency. |
actions/setup/js/pi_runtime.cjs |
Adds default retry settings. |
actions/setup/js/pi_runtime.test.cjs |
Verifies retry configuration. |
actions/setup/js/pi_provider.cjs |
Reports sanitized provider failures. |
actions/setup/js/pi_provider.test.cjs |
Tests failure diagnostics and redaction. |
actions/setup/js/pi_provider_error.cjs |
Implements bounded error sanitization. |
actions/setup/js/pi_agent_core_driver.cjs |
Sanitizes streamed error events. |
actions/setup/js/pi_agent_core_driver.test.cjs |
Tests event redaction. |
.github/workflows/agentic_commands.yml |
Regenerates command routing. |
.changeset/patch-harden-pi-provider-failures.md |
Documents the patch. |
| @@ -1,4 +1,4 @@ | |||
| # gh-aw-commands: {"payload_version":"v1","schema_version":"v1","compiler_version":"dev","commands":["*","ace","approach-validator","archie","cloclo","craft","dependabot-burner","grumpy","matt","mergefest","nit","plan","poem-bot","ponytail","review","ruflo","scout","security-review","smoke-agent-all-merged","smoke-agent-all-none","smoke-agent-public-approved","smoke-agent-public-none","smoke-agent-scoped-approved","smoke-aider","smoke-call-workflow","smoke-checkout-pr-dispatch","smoke-claude","smoke-claude-on-copilot","smoke-codex","smoke-copilot","smoke-copilot-aoai-apikey","smoke-copilot-aoai-entra","smoke-copilot-arm","smoke-copilot-mai","smoke-copilot-sdk","smoke-copilot-small","smoke-create-cross-repo-pr","smoke-crush","smoke-cursor","smoke-deepseek-harness","smoke-drive","smoke-gemini","smoke-github-claude","smoke-goose","smoke-kiro","smoke-multi-pr","smoke-opencode","smoke-otel-backends","smoke-pi","smoke-project","smoke-pydantic","smoke-service-ports","smoke-temporary-id","smoke-test-tools","smoke-update-cross-repo-pr","souschef","squad-plan","summarize","tidy","unbloat","windows"],"workflows":["ace-editor","approach-validator","archie","cloclo","craft","dependabot-burner","design-decision-gate","dev","grumpy-reviewer","mattpocock-skills-reviewer","mergefest","necromancer","pdf-summary","plan","poem-bot","ponytail-reviewer","pr-code-quality-reviewer","pr-nitpick-reviewer","pr-sous-chef","ruflo-backed-task","scout","security-review","skillet","smoke-agent-all-merged","smoke-agent-all-none","smoke-agent-public-approved","smoke-agent-public-none","smoke-agent-scoped-approved","smoke-aider","smoke-call-workflow","smoke-checkout-pr-dispatch","smoke-claude","smoke-claude-on-copilot","smoke-codex","smoke-copilot","smoke-copilot-aoai-apikey","smoke-copilot-aoai-entra","smoke-copilot-arm","smoke-copilot-mai","smoke-copilot-sdk","smoke-copilot-small","smoke-create-cross-repo-pr","smoke-crush","smoke-cursor","smoke-deepseek-harness","smoke-drive","smoke-gemini","smoke-github-claude","smoke-goose","smoke-kiro","smoke-multi-pr","smoke-opencode","smoke-otel-backends","smoke-pi","smoke-project","smoke-pydantic","smoke-service-ports","smoke-temporary-id","smoke-test-tools","smoke-update-cross-repo-pr","squad-plan","test-quality-sentinel","tidy","unbloat-docs","windows"]} | |||
| # gh-aw-commands: {"payload_version":"v1","schema_version":"v1","compiler_version":"dev","commands":["*","ace","approach-validator","archie","cloclo","craft","dependabot-burner","grumpy","mergefest","nit","plan","poem-bot","ruflo","scout","security-review","smoke-claude-on-copilot","smoke-copilot","smoke-copilot-aoai-apikey","smoke-copilot-aoai-entra","smoke-copilot-mai","smoke-copilot-sdk","smoke-copilot-small","smoke-github-claude","smoke-otel-backends","smoke-service-ports","souschef","squad-plan","summarize","tidy","unbloat","windows"],"workflows":["ace-editor","approach-validator","archie","cloclo","craft","dependabot-burner","dev","grumpy-reviewer","mergefest","necromancer","pdf-summary","plan","poem-bot","pr-nitpick-reviewer","pr-sous-chef","ruflo-backed-task","scout","security-review","skillet","smoke-claude-on-copilot","smoke-copilot","smoke-copilot-aoai-apikey","smoke-copilot-aoai-entra","smoke-copilot-mai","smoke-copilot-sdk","smoke-copilot-small","smoke-github-claude","smoke-otel-backends","smoke-service-ports","squad-plan","tidy","unbloat-docs","windows"]} | |||
There was a problem hiding this comment.
Restored the complete generated route table by restoring expression-capable queue validation and recompiling; /review, /ponytail, and /smoke-pi are present. All 320 workflows compile. Commit: 452c0a8.
| if (event.type !== "message_update") { | ||
| const sanitized = { ...event }; | ||
| if (typeof event.errorMessage === "string") sanitized.errorMessage = sanitizeProviderErrorMessage(event.errorMessage); | ||
| if (typeof event.message?.errorMessage === "string") { | ||
| sanitized.message = { ...event.message, errorMessage: sanitizeProviderErrorMessage(event.message.errorMessage) }; | ||
| } | ||
| return sanitized; | ||
| } |
There was a problem hiding this comment.
Sanitized assistant errorMessage fields in event.messages[] and added an agent_end regression test. Commit: 452c0a8.
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 PR mixes the Pi diagnostics work with unrelated routing and schema regressions that would break existing workflows and slash-command dispatch.
Blocking themes
agentic_commands.ymldrops a large set of still-present commands and workflows from routing.preparePiRuntime()adds retry defaults with a shallow merge, so existing retry config can nullify or partially erase the new behavior.github-workflow.jsonstops accepting expression-valuedconcurrency.queue, but the repository already ships many workflows that rely on that syntax.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 48.9 AIC · ⌖ 7.35 AIC · ⊞ 19.4K
Comment /review to run again
Comments that could not be inline-anchored
.github/workflows/agentic_commands.yml:147
This regeneration drops existing routes like /review, /matt, /ponytail, and multiple smoke workflows even though their workflow files are still in the repo, so merging this would silently break dispatch for commands that already exist.
<details><summary>💡 Why this blocks</summary>
The diff removes these entries from the generated command inventory and the slash-routing payload without deleting the corresponding workflow sources. That means the workflows still exist, but the router stop…
actions/setup/js/pi_runtime.cjs:101
This retry default is only shallow-merged, so any existing settings.json.retry object replaces it wholesale and can silently disable retries or drop the new delay caps.
<details><summary>💡 Why this blocks</summary>
preparePiRuntime() spreads installedSettings and then config.settings over the top-level object. If either source already contains retry, the entire default object you just added is overwritten instead of merged field by field. For example, an older install that only h…
pkg/workflow/schemas/github-workflow.json:46
Narrowing concurrency.queue to a literal enum breaks existing workflows that use ${{ ... }} expressions here, so schema validation will start rejecting workflows we already ship.
<details><summary>💡 Why this blocks</summary>
This repo already checks in many generated workflows with queue: ${{ github.event_name == "pull_request" && "single" || "max" }} such as pr-code-quality-reviewer.lock.yml, design-decision-gate.lock.yml, and multiple smoke workflows. After this change, those val…
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd. Requesting changes: the schema change in this PR regresses concurrency.queue expression support and fails existing tests on this branch.
📋 Key Themes & Highlights
Key Themes
- Blocking regression:
pkg/workflow/schemas/github-workflow.jsondrops theexpressionSyntaxalternative forconcurrency.queue, breakingTestConcurrencyQueueSchemaValidationand twoTestCommandConcurrencyCompilationsubtests (verified locally — all three fail on this branch). This appears unrelated to the Pi provider hardening goal and should be reverted or fixed. - Test coverage gap: the new
pi_provider_error.cjsredaction/parsing helpers are security-sensitive but only tested indirectly throughpi_provider.test.cjs/pi_agent_core_driver.test.cjs; a dedicated unit test file would pin down the regex edge cases directly.
Positive Highlights
- ✅
getProviderErrorDetails/sanitizeProviderErrorMessageare a clean, single-purpose extraction that keepspi_provider.cjsreadable. - ✅ Good defense-in-depth: both env-var-driven and pattern-based (Bearer,
ghp_,sk-...) secret redaction, plus a length cap. - ✅ New tests in
pi_provider.test.cjsandpi_agent_core_driver.test.cjsassert both that the useful diagnostic (status/message) is present and that the secret is absent — good regression-test shape per/tdd. - ✅
pi_runtime.cjsretry defaults (maxRetries: 2, capped backoff) are a reasonable, test-covered hardening default.
Unrelated to this PR's code: note that .github/workflows/agentic_commands.yml's diff appears to be a large routing-table regeneration (likely from gh aw compile/gh aw update), not reviewed in depth here.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 73.5 AIC · ⌖ 14.7 AIC · ⊞ 10K
Comment /matt to run again
| } | ||
| ] | ||
| }, | ||
| "queue": { |
There was a problem hiding this comment.
[/diagnosing-bugs] This diff removes the expressionSyntax alternative from concurrency.queue, regressing support for ${{ ... }} expressions (e.g. queue: ${{ github.event_name == 'pull_request' && 'single' || 'max' }} used by command-triggered workflows). This is unrelated to the Pi provider hardening goal of this PR and breaks existing coverage.
💡 Verification & fix
Running the existing suite against this branch fails:
go test ./pkg/workflow/... -run 'TestConcurrencyQueueSchemaValidation|TestCommandConcurrencyCompilation'
--- FAIL: TestCommandConcurrencyCompilation/centralized_command_and_PR
--- FAIL: TestCommandConcurrencyCompilation/label_command_and_PR
--- FAIL: TestConcurrencyQueueSchemaValidation
.../concurrency: 'oneOf' failed, none matched
- at '/concurrency/queue': value must be one of 'single', 'max'
pkg/workflow/concurrency_command_test.go expects queue: ${{ github.event_name == 'pull_request' && 'single' || 'max' }} to validate successfully (see TestCommandConcurrencyCompilation/centralized_command_and_PR and TestConcurrencyQueueSchemaValidation), which this schema no longer allows since the oneOf + expressionSyntax branch was dropped in favor of a plain string enum.
This looks like an accidental revert/leftover from an unrelated change swept into this PR — restore the oneOf with expressionSyntax so command-triggered workflows that compute queue dynamically keep validating.
@copilot please address this.
There was a problem hiding this comment.
Restored the enum-or-expression oneOf for concurrency.queue; the targeted queue-schema and command-concurrency Go tests pass. Commit: 452c0a8.
| message = value; | ||
| } else { | ||
| try { | ||
| message = JSON.stringify(value); |
There was a problem hiding this comment.
[/tdd] sanitizeProviderErrorMessage and getProviderErrorDetails are new, security-sensitive functions (credential redaction) but have no dedicated unit test file — coverage currently comes only indirectly via pi_provider.test.cjs and pi_agent_core_driver.test.cjs.
💡 Suggested tests
Add pi_provider_error.test.cjs covering edge cases directly, e.g.:
it("redacts Bearer tokens and GH tokens regardless of env vars", () => {
expect(sanitizeProviderErrorMessage("Authorization: Bearer abc123")).not.toContain("abc123");
expect(sanitizeProviderErrorMessage("ghp_abcdefghijklmnopqrstuvwxyz0123456789")).toContain("[REDACTED]");
});
it("truncates very long messages", () => {
const long = "x".repeat(2000);
expect(sanitizeProviderErrorMessage(long).length).toBeLessThanOrEqual(1001);
});
it("parses status prefix from getProviderErrorDetails when no responseStatus given", () => {
expect(getProviderErrorDetails("429: rate limited", undefined)).toEqual({ status: 429, message: "rate limited" });
});This makes regressions in the redaction regexes (e.g. the control-char/secret patterns) fail fast and close to the change, per /tdd.
@copilot please address this.
There was a problem hiding this comment.
Added dedicated tests for provider-error redaction, control-character removal, length bounding, and status parsing in pi_provider_error.test.cjs. The impacted setup JS suite passes. Commit: 452c0a8.
There was a problem hiding this comment.
Impeccable Review (harden mode) — verified existing findings
Ran the Impeccable harden + audit checks (bug-fix/reliability change). Three blocking issues were already flagged by a prior review pass on this PR; I independently reproduced and confirmed all three are real:
-
pkg/workflow/schemas/github-workflow.json(concurrency.queue) — dropping theexpressionSyntaxoneOfbranch breaks existing behavior. Confirmed by running:go test ./pkg/workflow/... -run 'TestConcurrencyQueueSchemaValidation|TestCommandConcurrencyCompilation'Both fail:
TestConcurrencyQueueSchemaValidationandTestCommandConcurrencyCompilation/centralized_command_and_PR+/label_command_and_PRerror withjsonschema validation failed ... value must be one of 'single', 'max'because the compiler still emits${{ github.event_name == 'pull_request' && 'single' || 'max' }}for mixed command/PR concurrency groups. This is a genuine regression, not a false positive. -
.github/workflows/agentic_commands.ymlrouting table — the regenerated routing table drops/review,/matt,/ponytail,/smoke-pi,/smoke-claude,/smoke-codex, and many othersmoke-*commands and workflows, even though their source workflow files are untouched by this PR. This file shouldn't be hand-regenerated as part of an unrelated Pi-retry change — looks like it was compiled against a stale/filtered workflow set. -
pi_agent_core_driver.cjs: jsonEvent— the sanitizer only redactsevent.errorMessageandevent.message.errorMessage, butagent_endevents carry errors inevent.messages[](plural, array) per the existing fixtures (fixtures/pi_ci_stream.cjs:58,84,pi_session.test.cjs). Any assistant error captured via that path is emitted to the session log/stdout unsanitized, which can leak provider secrets — directly undermining the stated goal of this PR ("sanitized response message in failure reports and logs").
No new issues beyond these found on review. Given issue (3) is a secret-leak regression in the very code this PR adds, and (1)/(2) are confirmed test failures / unrelated-file regressions, this PR should not merge until addressed.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 93.4 AIC · ⌖ 13.1 AIC · ⊞ 8.1K
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Finisher pass completed; pushed |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| const providerFailureDetails = lastProviderFailure | ||
| ? ` Last provider failure: ${typeof lastProviderFailure.status === "number" ? `HTTP ${lastProviderFailure.status}; ` : ""}message=${JSON.stringify(lastProviderFailure.message)}.` | ||
| : ""; | ||
| emitInfrastructureIncompleteIfNoSafeOutputs(`All ${providerRequestCount} Pi provider requests failed before safe outputs were emitted.${providerFailureDetails}`, log); |
|
🎉 This pull request is included in a new release. Release: |
Pi provider failures, including rate limits, could end as generic infrastructure errors without actionable response details.