Skip to content

fix: isolate credentialed PR evaluation from candidate code - #547

Merged
Paulo Lacerda (placerda) merged 4 commits into
developfrom
placerda-pr-gate-config-review
Nov 6, 2026
Merged

Paulo Lacerda (placerda) merged 4 commits into
developfrom
placerda-pr-gate-config-review

Conversation

@placerda

@placerda Paulo Lacerda (placerda) commented Nov 6, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Relates to #499. Keep automatic candidate prompt/dataset evaluation without executing PR-authored code or configuration with evaluation credentials.

  • Read immutable candidate Git blobs and import only JSONL data and plain prompt text into a trusted checkout. Reject all candidate configuration changes, including endpoints, authentication selectors, recipes, thresholds and bootstrap controls.
  • Classify Foundry hosts by parsed hostname boundaries, not Azure-looking text in paths or queries. Preserve explicit private-host protocols and sovereign Azure hosts.
  • Prevent credential-bearing HTTP redirects from crossing origins or downgrading HTTPS, including custom credentials carried in normally public header names.
  • Preserve the flat configuration schema, local explicit configuration behavior, normalized results and exit codes. Attribute trusted PR runs to the candidate source SHA.

Trusted execution and migration

GitHub templates use pull_request_target, check out the exact trusted base SHA and prepare candidate data before Azure login. An optional dedicated status-only GitHub App publishes agentops/pr-eval on the actual head SHA. The App must be selected as the expected status source. A shared GitHub Actions status is informational, not exclusive evaluator proof.

Azure Repos templates use a closed protected-ref extends template in the same repository. A separate repository is not mandatory. Literal protected checkouts avoid caller-controlled aliases. PR-only resources, pinned Git/API destinations, isolated installation and restricted task logging prevent candidate code/configuration from steering credentialed execution. Status publication uses the service connection's exclusive principal and verifies the current PR source SHA/iteration, not the shared Build Service identity.

Regenerate copied workflows from a release containing this helper. Configure the external protected-template/resource/identity controls in docs/ci-github-actions.md before granting PR credentials or treating results as secure merge proof. Nonsecret ADO group values require administrator-only updates. Any group secrets need independent checks that still reject malicious root YAML after it removes the checked connection. A connection check alone does not protect such secrets.

This remains separate from 0.16.1. The changelog entry is only under Unreleased. No release branch, tag, publication or merge was changed.

Validation

Baseline validation at b8df0114, before the cloud-output follow-up:

  • Full existing suite on updated develop: 1708 passed, 1 skipped.
  • Six changed runtime/configuration sources pass mypy with --follow-imports=silent.
  • New helper and tests pass Ruff. Changed tracked Python files introduce no additional Ruff diagnostics relative to their baseline.
  • Final template/documentation clarification: 96 generator tests passed.
  • Security regressions use synthetic configuration, dummy credentials, local Git fixtures and mocked HTTP/authentication. They cover destination/auth changes, data-only import, symlinks/path/size validation, moving revisions, protected ADO Git/API targets, status iteration races, logging-command injection and credential-safe JSON/streaming redirects.

ADO cloud-output follow-up

The focused review identified a functional fail-closed bug in the cloud preparation step's logging policy. The trusted Prepare AgentOps cloud eval config producer now explicitly permits only AGENTOPS_CI_CONFIG, without depending on an exit-code setter in the same task. Eval consumers can emit only AGENTOPS_EVAL_EXIT_CODE, not change the prepared config variable.

  • Both new generic and prompt-agent cloud matrix cases failed before the repair and pass after it. They verify the producer permission, its setter, the consumer's matching config reference, and retained candidate-output restrictions.
  • test_cicd.py plus test_pr_gate.py: 124 passed.
  • Changed generator passes mypy. No new Ruff findings relative to the reviewed head.
  • The full suite was not rerun after this surgical follow-up. The earlier full-suite result remains attributed to b8df0114.

Limitations and external acceptance checks

No real Azure credentials, paid evaluations or cloud mutations were used. Live ADO YAML expansion, protected-resource rejection and GitHub/ADO policy enforcement were not exercised because no external resources/settings were authorized. Administrators must verify the negative cases before adoption, especially removal of a checked connection while requesting a secret group and forged statuses from shared identities.

Candidate prompts/dataset rows still influence the agent and tools. Use a least-privilege sandbox with no production data or deployment rights. This change is not a general prompt-injection defense.

Preserve automatic data-only PR evaluation with trusted configuration, protected templates, credential-safe destinations, and revision-bound status provenance. Relates to #499.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Distinguish administrator-controlled nonsecret values from independently checked secret resources. Do not imply a service connection check protects a group after the connection is removed.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Grant AGENTOPS_CI_CONFIG only to the named trusted preparation step and restrict eval consumers to their exit-code output. Cover generic and prompt-agent PR cloud gates in the regression matrix.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

@placerda Paulo Lacerda (placerda) left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Integration review complete on 3ac5564 after a normal merge of current develop f98e873. Reviewed the runtime trust-boundary changes, immutable data-only preparation, both GitHub templates, protected ADO extends flow and output allowlists, migration documentation and regression coverage. The separate independent review reported no remaining high-confidence exploitable finding and verified the ADO cloud-output follow-up. Fresh full CI 42303875100 passed on this exact head. Local full suite on this head: 1710 passed, 1 expected conditional skip because MCP is installed. Focused changed-area suite: 297 passed. This is a review comment, not a fabricated self-approval. The user explicitly requested integration of this PR. No production release, external protected resources, exclusive status identity or Azure/ADO policy enforcement is claimed by this merge. Existing copied workflows require regeneration and documented administrator controls after a release containing the fix.

@placerda
Paulo Lacerda (placerda) merged commit 644a075 into develop Nov 6, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant