Repository navigation
Fix smoke experiment expressions and make OIDC reminders informational - #66928
Conversation
Resolve declared experiment references to activation-local step outputs without changing runtime declarations. Cover inline and imported agents, preserve static metadata, document support, and regenerate affected smoke workflows. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep id-token audience and cloud trust guidance without incrementing compiler warnings. Preserve OIDC permission enforcement, add strict/non-strict and development-gate regressions, and synchronize compiler threat detection specification 1.0.44, mappings, changelog, and permissions documentation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
✅ 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.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ 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 — ADR RequiredThis PR makes significant changes to core business logic (172 new lines across 📄 Draft ADR committed:
🔍 Evidence used
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Impeccable Skills Review
This PR is a Go compiler fix (experiment expression rewriting + OIDC permission diagnostic downgrade), not a UI/frontend change, so the installed Impeccable modes (audit, critique, harden, distill, extract, clarify) do not apply to the changed code.
Falling back to a standard correctness/security review per the fallback instructions:
RewriteExperimentsReferenceForDownstreamJobs/RewriteActivationOutputsToLocalStepOutputs(pkg/workflow/compiler_experiments.go) are regex-scoped to only declared experiment names, well-documented, and applied in the correct order insidewriteSubagentModelDeclarations, matching the activation-job context wheresteps.pick-experiment.outputsis valid (confirmed viagenerateCreateAwInfo).- The
id-token: writepermission change (pkg/workflow/permissions_compiler_validator.go) correctly downgrades the reminder toinfowithout incrementing the warning counter, consistent with updated tests (TestIdTokenWriteInfo,pull_request_target_validation_test.go) and docs. - New/updated tests cover inline/imported agents, compound expressions, diagnostic severity, and dev-gate behavior; lock files and specs are regenerated/updated consistently.
No correctness, security, or maintainability issues found in the changed lines.
@copilot no action needed.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 27.5 AIC · ⌖ 13 AIC · ⊞ 8.1K
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
There was a problem hiding this comment.
Verdict
I found one actionable correctness issue in the new experimental sub-agent metadata path.
Theme
- The activation-job metadata rewrite fixes the invalid
experiments.*context inmodel, but it still leaves experiment-backed sub-agents without matchablepatterns, which will produce false declared-model audit mismatches downstream.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 37.7 AIC · ⌖ 5.52 AIC · ⊞ 19.8K
Comment /review to run again
There was a problem hiding this comment.
🟡 Changes recommended
Compound experiment expressions remain invalid at runtime, and experiment aliases are not expanded for metadata auditing or model-routing policy.
2 open findings
What changed in this PR
Fixes smoke workflow experiment metadata and downgrades OIDC reminders from warnings to informational diagnostics.
Changes:
- Rewrites sub-agent experiment references for activation metadata.
- Makes
id-token: writereminders informational. - Adds tests, documentation, specification updates, and regenerated locks.
| File | Description |
|---|---|
specs/compiler-threat-detection-spec.md |
Updates CTR-001 requirements. |
specs/compiler-threat-detection-changelog.md |
Records specification 1.0.44. |
pkg/workflow/pull_request_target_validation_test.go |
Updates OIDC warning expectations. |
pkg/workflow/permissions_compiler_validator.go |
Emits OIDC reminder as info. |
pkg/workflow/idtoken_write_info_test.go |
Tests informational diagnostics. |
pkg/workflow/compiler_yaml_step_lifecycle.go |
Rewrites sub-agent metadata expressions. |
pkg/workflow/compiler_threat_detection_formal_test.go |
Adds CTR-001 conformance coverage. |
pkg/workflow/compiler_experiments_test.go |
Tests experiment-backed sub-agent metadata. |
pkg/cli/compile_development_test.go |
Verifies dry-run acceptance. |
docs/src/content/docs/reference/permissions.md |
Documents OIDC severity behavior. |
docs/src/content/docs/experimental/experiments.md |
Documents sub-agent model experiments. |
.github/workflows/smoke-copilot.lock.yml |
Regenerates smoke metadata expression. |
.github/workflows/smoke-copilot-aoai-entra.lock.yml |
Regenerates Entra smoke lock. |
.github/workflows/smoke-copilot-aoai-apikey.lock.yml |
Regenerates API-key smoke lock. |
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd (per pr-triage classification: bug_fix). No actionable issues found — approving.
📋 Analysis
Root-cause fix verified
writeSubagentModelDeclarations(compiler_yaml_step_lifecycle.go) correctly chainsRewriteExperimentsReferenceForDownstreamJobs→RewriteActivationOutputsToLocalStepOutputsso${{ experiments.subagent_model }}becomes${{ steps.pick-experiment.outputs.subagent_model }}for metadata evaluated inside the activation job, while the runtimedata.SubAgentModelsdeclaration is left unmutated (verified by the explicit "must not mutate" assertion inTestWriteSubagentModelDeclarationsExperimentExpressions). This is the actual root cause fix, not a symptom patch, since the aw_info step runs in the same job aspick-experimentand can't useneeds.activation.outputs.*on itself.- Confirmed via
git diff HEAD~1 -- .github/workflows/smoke-copilot.lock.ymlthat the regenerated lock now emits the correctedsteps.pick-experiment.outputs.subagent_modelexpression.
OIDC reminder downgrade
permissions_compiler_validator.gochange fromwarning→infoand removal ofc.IncrementWarningCount()is consistent with the existinginfodiagnostic pattern used elsewhere (e.g.create_discussion.go, redirect notices), and doesn't weaken CTR-001 enforcement — missing/invalid OIDC permissions remain errors.pull_request_target_validation_test.go's "unrelated warning remains" case was correctly updated to "id-token permission is informational" rather than silently dropped.
Test coverage (/tdd)
- New tests cover inline and imported sub-agents, compound
format(...)/ fallback (||) expressions, undeclared-experiment names, and ordering (pick-experimentstep precedesgenerate_aw_info) — good edge-case coverage per TDD principles. TestFormal_CTR001_IDTokenWriteIsInformationalcovers both strict and non-strict modes and asserts the warning count stays at zero.
Docs & spec
specs/compiler-threat-detection-spec.md/changelog version bump to 1.0.44 and CTR-001 rule text are consistent with the code change.experiments.mddoc addition aboutsubagent_modeland the Pi literal-model restriction accurately matchesvalidatePiSubagentModel'scontainsExpressioncheck.
Positive highlights
- ✅ Clear root-cause fix (rewrites metadata reference, not a special-case workaround)
- ✅ Behavior-preserving: runtime declarations untouched, only generated metadata rewritten
- ✅ Spec/changelog/docs kept in sync with the code and test changes
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 85.5 AIC · ⌖ 14.4 AIC · ⊞ 10.1K
Comment /matt to run again
Reject compound and undeclared experimental sub-agent models rather than accepting runtime expressions the sub-agent renderer cannot evaluate. Share concrete variant expansion between metadata and routing policy, preserve router candidates, and complete the ADR with explicit pending maintainer acceptance. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@copilot address the following outstanding work in one pass:
Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI. Sous-chef head: feceadc
|
|
🎉 This pull request is included in a new release. Release: |
Why
Three Copilot smoke workflows are rejected before any jobs start because generated sub-agent metadata contains the gh-aw-only
experiments.subagent_modelcontext. Separately, the intentionalid-token: writepermission in the Entra workflow produces a warning that contributes to warnings-as-errors failures.Changes
steps.pick-experiment.outputs, since that metadata is evaluated inside activation. Preserve authored runtime declarations and regenerate the three affected smoke locks.models.allowed/models.blockedfiltering and router candidates. These union patterns indicate possible model usage, not proof that the selected variant was honored or delegation occurred.${{ experiments.<name> }}with complete models or aliases as variants.infowithout incrementing compiler warnings. Keep invalid permission values and missing required OIDC permissions as errors.ADR: ADR-66928: Scope Experiment Expressions to Their Evaluating Job and Keep Security Reminders Advisory. The record is complete; maintainer acceptance remains part of PR review.
Validation
make build,make fmt,make lint, and focused workflow/CLI unit regressions covering the review fixes.test-unitcoverage withinmake agent-report-progress.go test -tags integration ./pkg/workflow -run '^TestIdTokenWriteInfo' -count=1on the unchanged OIDC implementation before this finisher pass.make check-workflow-drift; all 330 workflow locks remain synchronized with the final compiler.Known pre-existing blockers remain unchanged: the progress gate reports unchecked
fmt.Fprintf/fmt.Fprintlnresults and the existing oversized permissions validator; the combined smoke strict dry-run still reports one other warning; pinned actionlint rejects the existingconcurrency.queuesyntax. The Entra OIDC reminder now appears as info.CI on the final pushed HEAD is unverified. A maintainer must re-trigger CI before merge; no workflow run or merge was initiated by this finisher pass.