fix(#880): resolve retro agent provider credential conflict

[maruiz93]

Summary

• Reverts providers/github-artifacts.yaml from GH_TOKEN back to _NOOP_GITHUB_ARTIFACTS: "" — the credential-less noop pattern used by other network-policy-only providers (vertex-ai, gitleaks, package-registries)
• The artifacts provider only needs network policy rules; the API call to api.github.com is authenticated by the companion github-ro provider, and redirect targets (Azure Blob Storage) use pre-signed SAS URLs that need no Authorization header
• Adds an explanatory comment on the provider documenting why GH_TOKEN must not be declared here
• Adds a retro smoke eval (eval/retro/) that verifies sandbox creation succeeds — directly validates the fix by catching credential key conflicts at sandbox startup

Fixes #880.

Context

PR #211 changed github-artifacts.yaml from _NOOP_GITHUB_ARTIFACTS: "" to GH_TOKEN: "${GH_TOKEN}". This caused OpenShell to reject sandboxes where two providers (github-ro and github-artifacts) both declare GH_TOKEN as their credential key. The retro agent is the only agent currently attaching both providers, so it was the only one affected.

The fix is reverting to the original noop credential. No combined provider/profile is needed — the artifact download flow works without credentials on the artifacts provider because:

1. The initial API call to api.github.com goes through github-ro, which injects GH_TOKEN
2. GitHub returns a 302 redirect to Azure Blob Storage with a SAS token in the URL
3. The gh CLI (and OpenShell's proxy) drops the Authorization header on cross-domain redirects
4. The SAS-signed URL authenticates the download — no GH_TOKEN needed

Test plan

• Retro agent sandbox creation succeeds (no more credential env key 'GH_TOKEN' is provided by both provider error)
• functional-tests (retro) CI green — sandbox_started judge 100%
• Retro agent can access both GitHub API and artifact download endpoints
• Other agents (review, triage, prioritize, code, fix) unaffected — their harnesses are unchanged

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Resolve retro GH_TOKEN conflict with a combined GitHub provider

🐞 Bug fix 🧪 Tests ⚙️ Configuration changes 🕐 20-40 Minutes

AI Description

• Combine GitHub API and artifact access under one GH_TOKEN provider for retro.
• Rewire retro harness to prevent duplicate credential-key rejection during sandbox creation.
• Add a functional smoke evaluation that verifies the retro sandbox starts.

Diagram

graph TD
  E["Retro smoke eval"] --> R["Eval runner"] --> H["Retro harness"] --> P["Combined provider"] --> F["Combined profile"] --> G["GitHub API"]
  F --> A["Artifact hosts"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Upstream shared credentials

• ➕ Preserves independently maintained provider profiles.
• ➕ Eliminates duplicated endpoint definitions.
• ➕ Supports future provider combinations without bespoke profiles.
• ➖ Requires an OpenShell capability that is not currently available.
• ➖ Cannot immediately restore retro sandbox creation.

2. Expand the existing read-only profile

• ➕ Avoids introducing another provider and profile pair.
• ➕ Keeps endpoint definitions in one existing profile.
• ➖ Grants artifact-host access to agents that only need the GitHub API.
• ➖ Broadens the semantics and permissions of an established profile.

Recommendation: Use the combined provider as the scoped short-term fix because it restores retro without changing permissions for other agents. Pursue upstream credential reuse as the long-term solution, since manually mirrored endpoint lists create an ongoing synchronization burden.

Files changed (10) +171 / -4

Bug fix (2) +7 / -4

retro.yamlSwitch retro to the combined GitHub provider +2/-4

Switch retro to the combined GitHub provider

• Replaces the separate read-only and artifact providers and profiles with their combined equivalents, ensuring GH_TOKEN is declared only once.

harness/retro.yaml

github-ro-artifacts.yamlDeclare a single-credential combined provider +5/-0

Declare a single-credential combined provider

• Adds the github-ro-artifacts provider backed by the combined profile and one GH_TOKEN credential declaration.

providers/github-ro-artifacts.yaml

Tests (5) +120 / -0

annotations.yamlDefine the retro smoke-test expectation +4/-0

Define the retro smoke-test expectation

• Marks the fixture as open and documents that only successful sandbox startup is evaluated, not completion of the synthetic retro task.

eval/retro/cases/001-smoke-sandbox/annotations.yaml

input.yamlAdd a pull-request fixture for retro evaluation +19/-0

Add a pull-request fixture for retro evaluation

• Defines a synthetic GitHub pull request that supplies a valid originating URL for the retro harness smoke test.

eval/retro/cases/001-smoke-sandbox/input.yaml

README.mdAdd the smoke fixture repository content +3/-0

Add the smoke fixture repository content

• Provides the base README used to construct the ephemeral repository for the retro evaluation case.

eval/retro/cases/001-smoke-sandbox/repo/README.md

eval.yamlIntroduce a retro sandbox smoke evaluation +89/-0

Introduce a retro sandbox smoke evaluation

• Configures fixture lifecycle hooks, the retro CLI runner, and required credentials. Adds a judge that requires metrics from at least one agent turn, directly detecting sandbox creation failures.

eval/retro/eval.yaml

run-fullsend.shPass retro fixture context into evaluation runs +5/-0

Pass retro fixture context into evaluation runs

• Writes the fixture URL as ORIGINATING_URL and forwards an optional RETRO_COMMENT when running the retro agent under evaluation.

eval/scripts/run-fullsend.sh

Other (3) +44 / -0

fullsend-github-artifacts.yamlDocument combined-profile synchronization +1/-0

Document combined-profile synchronization

• Adds a maintenance note indicating that artifact endpoints are mirrored by the combined read-only and artifacts profile.

profiles/fullsend-github-artifacts.yaml

fullsend-github-ro-artifacts.yamlCombine GitHub API and artifact permissions +42/-0

Combine GitHub API and artifact permissions

• Introduces a source-control profile containing the existing read-only REST, GraphQL, web, Azure Blob, and GitHub Actions endpoints. It permits both gh and node while retaining enforced read-only access.

profiles/fullsend-github-ro-artifacts.yaml

fullsend-github-ro.yamlDocument combined-profile synchronization +1/-0

Document combined-profile synchronization

• Adds a maintenance note indicating that the read-only API endpoints are mirrored by the combined profile.

profiles/fullsend-github-ro.yaml

[maruiz93]

working on adding some tests... to check if this actually fixes the bug it's related with

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 1:46 PM UTC · Ended 1:56 PM UTC

Commit: b80c31f · View workflow run →

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. Protected harness requires human review ✗ Dismissed 📜 Skill insight § Compliance

Description

This PR modifies the protected infrastructure file harness/retro.yaml. Although issue #880
justifies the change, the protected-path rule requires human approval.

Code

harness/retro.yaml[45]

+      - providers/github-ro-artifacts.yaml

Relevance

●●● Strong

Protected harness path changes require human review; matches prior protected-path governance rule
precedent.

PR-#631

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The changed line replaces the Retro agent's provider inside the protected harness/ path; PR
Compliance ID 1538392 requires a finding whenever such a path is modified.

harness/retro.yaml[45-48]
Skill: pr-review

---

2. Provider change lacks regression test ⊘ Outdated 📜 Skill insight ▣ Testability

Description

The runtime provider and profile selection changes without a corresponding automated test. The
unchecked manual test plan does not constrain sandbox creation or verify both GitHub API and
artifact access.

Code

harness/retro.yaml[R45-48]

+      - providers/github-ro-artifacts.yaml
    openshell:
      profiles:
-        - profiles/fullsend-github-ro.yaml
-        - profiles/fullsend-github-artifacts.yaml
+        - profiles/fullsend-github-ro-artifacts.yaml

Relevance

●● Moderate

Plausible testability gap but no clear team precedent enforcing regression tests for config-only
harness changes.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The cited configuration replaces two runtime providers and profiles with one combined provider and
profile, while the supplied diff contains no corresponding test change as required by PR Compliance
ID 1538339.

harness/retro.yaml[45-48]
Skill: code-implementation

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The Retro harness now selects a combined GitHub provider and profile, but the PR adds no automated regression test for the changed behavior.

## Issue Context
Coverage should verify that the configuration resolves without duplicate `GH_TOKEN` providers and retains both GitHub API and artifact endpoint access.

## Fix Focus Areas
- harness/retro.yaml[45-48]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Read-only sync comment stands alone ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

The sync comment is added while the associated fullsend-github-ro profile definition remains
functionally unchanged. This is a comment-only modification prohibited by the checklist.

Code

profiles/fullsend-github-ro.yaml[3]

+# Also mirrored by fullsend-github-ro-artifacts.yaml — keep in sync.

Relevance

●●● Strong

Active rule explicitly bans standalone comment-only changes without functional edits in the same
block.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Line 3 adds only a synchronization comment; the surrounding comments and profile fields are
unchanged. PR Compliance ID 1540690 requires comment changes to accompany a functional change in the
associated block.

Rule 1540690: Do not change comments on lines without functional code changes in a diff
profiles/fullsend-github-ro.yaml[2-5]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A comment was added to an otherwise functionally unchanged profile.

## Issue Context
PR Compliance ID 1540690 disallows comment-only changes when the associated code is identical before and after the change.

## Fix Focus Areas
- profiles/fullsend-github-ro.yaml[3-3]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Artifacts sync comment stands alone ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

The sync comment is added while the associated fullsend-github-artifacts profile definition
remains functionally unchanged. This is a comment-only modification prohibited by the checklist.

Code

profiles/fullsend-github-artifacts.yaml[2]

+# Also mirrored by fullsend-github-ro-artifacts.yaml — keep in sync.

Relevance

●●● Strong

Active rule explicitly bans standalone comment-only changes without functional edits in the same
block.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Line 2 adds only a synchronization comment; the neighboring profile fields are unchanged. PR
Compliance ID 1540690 requires comment changes to accompany a functional change in the associated
block.

Rule 1540690: Do not change comments on lines without functional code changes in a diff
profiles/fullsend-github-artifacts.yaml[2-3]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A comment was added to an otherwise functionally unchanged profile.

## Issue Context
PR Compliance ID 1540690 disallows comment-only changes when the associated code is identical before and after the change.

## Fix Focus Areas
- profiles/fullsend-github-artifacts.yaml[2-2]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context sources

✅ Compliance rules (platform): 55 rules

✅ Skills: 4 invoked

  code-review

  code-implementation

  pr-review

  docs-review

Tip of the day

💡 Did you know, you can copy the agent prompt from any finding and feed it to your IDE agent

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 1:58 PM UTC · Ended 2:00 PM UTC

Commit: 21e695b · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:01 PM UTC · Completed 2:17 PM UTC

Commit: 21e695b · View workflow run →

[qodo-code-review]

Code review by qodo was updated up to the latest commit 21e695b

[fullsend-ai-review]

Review

Findings

Medium

• [protected-path] providers/github-artifacts.yaml — This PR modifies a file under a protected path (providers/). The PR links to issue bug: retro agent sandbox creation fails — duplicate GH_TOKEN credential key across providers #880 and explains the rationale (reverting credential key from GH_TOKEN to _NOOP_GITHUB_ARTIFACTS to resolve the retro agent sandbox creation failure). Human approval is always required for protected-path changes, regardless of context.

Low

• [edge-case] eval/retro/eval.yaml:28 — The execution.timeout (600s) and EVAL_TIMEOUT (540s) give a tight 60-second gap. If sandbox creation stalls near the 9-minute mark (which the inline comment acknowledges can happen in merge queue runners), the test will fail with a generic timeout rather than a descriptive error. This is a deliberate design choice but worth noting for future debugging.

Previous run

Review

Findings

Medium

• [protected-path] providers/github-artifacts.yaml — This file is under the providers/ protected path. The PR links to issue bug: retro agent sandbox creation fails — duplicate GH_TOKEN credential key across providers #880 and explains the rationale (reverting the credential key to fix a duplicate GH_TOKEN conflict that broke retro agent sandbox creation), providing sufficient context. Human approval is always required for protected-path changes, regardless of context.

Low

• [edge-case] eval/retro/eval.yaml:8 — The execution.timeout (300s) leaves only 60 seconds of headroom above EVAL_TIMEOUT (240s). The before_each hook has a 120-second timeout and the two after_each hooks add another 60 seconds. Since this is a smoke test expected to fail fast, the risk is minimal.

• [scope-creep] eval/README.md:4 — The README diff adds fix/ alongside retro/ to the eval directory listing. The fix/ eval directory already existed on main — this corrects a pre-existing documentation gap rather than introducing scope beyond issue bug: retro agent sandbox creation fails — duplicate GH_TOKEN credential key across providers #880. Negligible.

Previous run (2)

Review

Findings

Medium

• [protected-path] providers/github-artifacts.yaml — This PR modifies a file under the protected path providers/. The change reverts the credential key from GH_TOKEN to _NOOP_GITHUB_ARTIFACTS to fix issue bug: retro agent sandbox creation fails — duplicate GH_TOKEN credential key across providers #880 (duplicate credential key conflict preventing retro agent sandbox creation). The PR links to issue bug: retro agent sandbox creation fails — duplicate GH_TOKEN credential key across providers #880 and the rationale is well-documented in the inline comment. Human approval is always required for protected-path changes, regardless of context.

Low

• [error-handling] eval/scripts/run-fullsend.sh:207 — The retro block emits RETRO_COMMENT with an empty default (${RETRO_COMMENT:-}). The eval harness does not currently provide a mechanism to populate RETRO_COMMENT from input.yaml via setup-fixture.sh, so future retro eval cases cannot exercise the on-demand retro path without additional plumbing. Not a bug for the current smoke test.

• [edge-case] eval/retro/cases/001-smoke-sandbox/annotations.yaml:7 — max_cost_usd is set to 2.00 with claude-opus-4-6 as the skill model. Budget may be tight if the agent retries before failing, though max_turns: 15 provides a stronger safeguard.

• [scope-creep] eval/retro/eval.yaml — The eval/retro/ directory is a net-new eval harness bundled with the bug fix. The smoke test directly validates the fix (sandbox creation succeeds without credential conflicts), making it a reasonable verification, though structurally it is a testing infrastructure addition beyond what issue bug: retro agent sandbox creation fails — duplicate GH_TOKEN credential key across providers #880 strictly authorizes.

Previous run (3)

Review

Findings

Medium

• [protected-path] providers/github-artifacts.yaml — This file is under the providers/ protected path. The PR links to issue bug: retro agent sandbox creation fails — duplicate GH_TOKEN credential key across providers #880 and explains the rationale for reverting the credential key (resolving a duplicate GH_TOKEN conflict for the retro agent), but human approval is always required for protected-path changes regardless of context.

Low

• [scope-creep] eval/retro/eval.yaml:1 — Issue bug: retro agent sandbox creation fails — duplicate GH_TOKEN credential key across providers #880 authorizes reverting the credential key in github-artifacts.yaml. The addition of the retro eval harness (eval.yaml, smoke test case, runner integration in run-fullsend.sh, README update) extends beyond the literal bug-fix scope. The eval directly validates the fix, which is good engineering practice, but the scope expansion should be acknowledged by a reviewer.

Info

• [provenance-warning] — Prior review context discarded: provenance validation failed (unverifiable-wrong-app). This review treats all findings as first-time assessments.

Previous run (4)

Review

Findings

Medium

• [protected-path] harness/retro.yaml, profiles/fullsend-github-artifacts.yaml, profiles/fullsend-github-ro-artifacts.yaml, profiles/fullsend-github-ro.yaml, providers/github-ro-artifacts.yaml — This PR modifies files under protected paths (harness/, profiles/, providers/). The PR links to issue bug: retro agent sandbox creation fails — duplicate GH_TOKEN credential key across providers #880 and explains the rationale for the changes (resolving a GH_TOKEN credential key conflict in the retro agent's sandbox). Human approval is always required for protected-path changes, regardless of context.

Low

• [edge-case] eval/retro/eval.yaml:82 — The forbidden_labels judge vacuously passes for the smoke test case because annotations.yaml has no labels.forbidden key. The judge returns True, "No forbidden labels specified" without exercising any assertion logic. Not a bug for the current smoke test, but the judge provides no value as-is — it will only be meaningful when a future case adds labels.forbidden.

• [scope-creep] eval/retro/eval.yaml — The eval/retro/ directory and supporting changes to eval/scripts/run-fullsend.sh and eval/README.md add eval infrastructure beyond issue bug: retro agent sandbox creation fails — duplicate GH_TOKEN credential key across providers #880's stated scope. However, the smoke test directly validates the fix by checking that sandbox creation succeeds (the exact failure mode of bug: retro agent sandbox creation fails — duplicate GH_TOKEN credential key across providers #880), making it a functional regression test for the core change.

• [architectural-coherence] providers/github-ro-artifacts.yaml — The combined provider/profile pattern (github-ro-artifacts) is novel in this codebase — all other providers and profiles are single-concern. The PR body acknowledges this is a short-term fix pending upstream support for credential reuse.

Previous run (5)

Review

Findings

Medium

• [protected-path] harness/retro.yaml, profiles/fullsend-github-artifacts.yaml, profiles/fullsend-github-ro-artifacts.yaml, profiles/fullsend-github-ro.yaml, providers/github-ro-artifacts.yaml — This PR modifies 5 files under protected paths (harness/, profiles/, providers/). The PR links to issue bug: retro agent sandbox creation fails — duplicate GH_TOKEN credential key across providers #880 and the description explains the rationale (resolving duplicate GH_TOKEN credential key conflicts in the retro agent sandbox). Human approval is always required for protected-path changes, regardless of context.

Low

• [edge-case] eval/retro/eval.yaml:72 — The sandbox_started judge uses metrics.get("num_turns", 0) but if num_turns is present in the JSON as null, Python's dict.get returns None (not the default 0), and int(None) raises a TypeError. The max_turns judge at line 102 handles this correctly with an explicit None check.
  Remediation: Add if turns is None: return False, "num_turns is null" or use turns = metrics.get("num_turns") or 0.

• [architectural-coherence] profiles/fullsend-github-ro-artifacts.yaml:12 — The combined profile uses category: source_control, but the original fullsend-github-artifacts.yaml uses category: data. This reclassifies artifact endpoints from data to source_control. Confirm no downstream tooling filters or routes by profile category.

• [stale-reference] eval/README.md:4 — The eval README lists agent eval directories as triage/, review/, code/ but this PR adds eval/retro/. The directory list should be updated to include retro/.

Previous run (6)

Review

Findings

Medium

• [protected-path] harness/retro.yaml, profiles/fullsend-github-artifacts.yaml, profiles/fullsend-github-ro-artifacts.yaml, profiles/fullsend-github-ro.yaml, providers/github-ro-artifacts.yaml — This PR modifies files under governance-protected paths (harness/, profiles/, providers/). The PR links to issue bug: retro agent sandbox creation fails — duplicate GH_TOKEN credential key across providers #880 and explains the rationale (resolving a credential key conflict that blocks sandbox creation), which provides sufficient context. Human approval is always required for protected-path changes regardless of context.

• [structural-inconsistency] eval/retro/eval.yaml:38 — The runner.env block (lines 38–44) duplicates keys already in execution.env (lines 11–17). For runner: type: cli, the harness builds the subprocess env from config.execution.env only; runner.env has no effect. This is dead configuration that could confuse future maintainers.
  Remediation: Remove the runner.env block to match eval/fix/eval.yaml and eval/code/eval.yaml, or add a comment explaining it is advisory-only.

Low

• [scope-creep] eval/retro/eval.yaml — The eval suite (eval/retro/) is not mentioned in issue bug: retro agent sandbox creation fails — duplicate GH_TOKEN credential key across providers #880, whose scope is the sandbox creation failure. The smoke test directly validates the fix, so the scope expansion is mild, but the PR description does not mention the eval addition.
  Remediation: Note the eval addition in the PR description as intentional scope expansion for verification.

---

Labels: PR fixes a retro agent bug and touches harness/provider/profile infrastructure

[waynesun09]

Review squad pass (Claude ×2, Grok) — verified against the live repo and CI. Excluding the existing HIGH protected-path finding since that's a human-review item. 5 unique MEDIUM findings posted inline; one more below that has no attachable file/line:

[MEDIUM] functional-tests (triage) is currently failing on this PR's head commit (21e695b) — case 001-bug-url-encoding fails, a suite unrelated to anything this PR touches. It appears to be a pre-existing flake, surfaced only because this PR's comment-only edits to profiles/fullsend-github-ro.yaml/fullsend-github-artifacts.yaml cause CI's changed-file matching to also select the triage suite. Worth confirming this is tracked separately before merge, since it's currently a red check on this PR for an unrelated reason.

For context: the core fix is verified working — functional-tests (retro) ran green in CI (232s, sandbox_started judge 100%), confirming issue #880's literal failure mode is fixed.

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 2:38 PM UTC · Ended 2:51 PM UTC

Commit: 0d6fe33 · View workflow run →

[maruiz93]

Updated the PR description to call out the eval addition as intentional scope expansion — it directly validates the fix by catching credential key conflicts at sandbox startup.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:53 PM UTC · Completed 3:12 PM UTC

Commit: a10e588 · View workflow run →

[waynesun09]

Follow-up from the grok re-review pass on commit 21e695b+0d6fe33: 2 new MEDIUM findings surfaced by the follow-up commit itself (eval judges/budgets), posted inline. These are independent of the node-widening thread already being discussed elsewhere in this PR.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:17 PM UTC · Completed 3:34 PM UTC

Commit: 919fbe3 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:39 PM UTC · Completed 3:56 PM UTC

Commit: 52cf2b6 · View workflow run →

[waynesun09]

LGTM at 52cf2b6. The pivot to _NOOP_GITHUB_ARTIFACTS restores the two-provider/two-profile pairing, keeps the Azure/actions artifact hosts gh-only (no node widening), matches the fullsend scaffold, and the EVAL_TIMEOUT fix closes the inner/outer timeout inversion. Remaining bot threads are low-severity and answered. Only outstanding signal is functional-tests (retro) on this head — should land green before merge.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:17 AM UTC · Completed 7:33 AM UTC

Commit: a06827d · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 7:44 AM UTC · Completed 8:01 AM UTC

Commit: 3c233d1 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:34 AM UTC · Completed 8:50 AM UTC

Commit: 61116aa · View workflow run →

[fullsend-ai-retro]

🤖 Finished Retro · ❌ Failure · Started 9:05 AM UTC · Completed 9:05 AM UTC

Commit: 61116aa · View workflow run →