Repository navigation
Show models powering auto in attribution footers - #74985
Conversation
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Design Decision Gate 🏗️Outcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
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.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Found one test-isolation issue; production attribution logic otherwise looks consistent with the added coverage.
Highlights
- The parameterized footer test mutates process-wide environment variables and does not restore them, creating order-dependent tests.
- The specialist pass completed; its other candidates were discarded as unsupported by the surrounding validated data flow or non-actionable.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · ~3 AIC · ⌖ ~1 AIC · ⊞ 1.6K
Comment /review to run again
| process.env.GH_AW_AGENT_AIC = "14"; | ||
| process.env.GH_AW_THREAT_DETECTION_AIC = "1"; | ||
| process.env.GH_AW_AMBIENT_CONTEXT = "9200"; | ||
| if (custom) process.env.GH_AW_SAFE_OUTPUT_MESSAGES = JSON.stringify({ footer: "> Generated by [{workflow_name}]({run_url}){ai_credits_suffix}{history_link}" }); |
There was a problem hiding this comment.
This test leaks process-wide environment variables into subsequent tests, making footer assertions order-dependent and potentially causing unrelated tests to use the custom template and deleted temp directory.
💡 Restore the environment after each parameterized case
Snapshot the relevant process.env keys before mutation and restore or delete them in finally (including GH_AW_SAFE_OUTPUT_MESSAGES, GH_AW_TMP_DIR, GH_AW_ENGINE_ID, GH_AW_ENGINE_MODEL, and the AIC/context keys). The current cleanup only removes the temporary directory, so the custom=true iteration leaves the custom footer configuration active for the rest of the file.
There was a problem hiding this comment.
Fixed in e7842a3: the footer test now snapshots and restores every environment variable it mutates, including the template, temp directory, engine, and AIC/context values.
There was a problem hiding this comment.
🟡 Changes recommended
Model attribution still includes failed-request models and loses earlier native model observations in Copilot auto runs without firewall usage, producing inaccurate model lists.
2 open issues
What changed in this PR
This PR updates gh-aw attribution footers to show the observed models powering auto selections.
Changes:
- Adds readable, deduplicated model names to
autolabels while preserving routing labels. - Filters classifier, detection, and untrusted events from model evidence.
- Adds regression coverage for model evidence and default/custom footer templates.
| File | Description |
|---|---|
actions/setup/js/model_attribution.test.cjs |
Tests model labels, deduplication, and evidence filtering. |
actions/setup/js/model_attribution.cjs |
Builds auto (powered by …) labels from observed models. |
actions/setup/js/messages.test.cjs |
Tests attribution in default and custom footers. |
Review effort: Balanced (auto)
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design. Requesting changes: the dedupe is ineffective for name variants, and a fallback-only label changed without being documented or tested.
📋 Key Themes & Highlights
Key Themes
- Dedupe runs before formatting: variant IDs that format to the same name still repeat in the footer.
- Empty name passes validation:
provider/yieldsauto (powered by ). - Unasserted behavior change: fallback-only
autonow labels asauto (powered by GPT 5.4)instead of the old alias. The PR body says fallback labels are unchanged. - Render timing unverified: the new branch depends on
aw_session.jsonl, which the code comments say may not exist yet when the agent post-step runs.
Positive Highlights
- ✅ Session records are filtered by provenance and purpose, so classifier, detection, and forged events are excluded.
- ✅ Models are validated before reaching the footer, and the test table covers invalid input.
- ✅ The footer-level test exercises both the default and custom templates.
Verification gap
Vitest could not be installed in this runner (npm rejected the registry certificate), so I did not run the test suite. The findings come from direct Node probes of getEffectiveModelLabel and from reading the code.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · routed: haiku55 xhigh · ~5 AIC · ⌖ ~1 AIC · ⊞ 5.4K
Comment /matt to run again
| } | ||
| } | ||
| if (!models.size && attribution.model && attribution.model.toLowerCase() !== "auto") models.add(attribution.model); | ||
| return models.size ? `auto (powered by ${[...models].map(formatAttributionModelName).join(", ")})` : "auto"; |
There was a problem hiding this comment.
[/diagnosing-bugs] Duplicate names slip past the dedupe. models is keyed on the raw identifier, but formatting runs afterwards. Probing google/gemini-3.8-flash, gemini-3.8-flash, and Gemini-3.8-Flash yields auto (powered by Gemini 3.8 Flash, Gemini 3.8 Flash, Gemini 3.8 Flash).
💡 Suggested fix
Format first, then dedupe the display names and drop empty ones:
const names = [...new Set([...models].map(formatAttributionModelName).filter(Boolean))];
return names.length ? `auto (powered by ${names.join(", ")})` : "auto";Add a regression case for these variants in model_attribution.test.cjs.
@copilot please address this.
There was a problem hiding this comment.
Fixed in e7842a3: model names are now formatted before deduplication, collapsing provider and case variants. Regression coverage is included.
| // Keep the full model version and tier readable rather than compressing them into an alias. | ||
| function formatAttributionModelName(model) { | ||
| return model | ||
| .replace(/^.*\//, "") |
There was a problem hiding this comment.
[/tdd] Empty model names reach the footer. provider/ passes validateModelIdentifier (it allows /), and formatAttributionModelName reduces it to an empty string, so the output is auto (powered by ).
💡 Suggested test
Use effective_model: "provider/" and expect "auto". The filter in the suggested fix above also covers this case.
@copilot please address this.
There was a problem hiding this comment.
Fixed in e7842a3: empty formatted names are discarded, so an identifier such as provider/ leaves the label as auto.
| const { reduceModelNameToIdentifier } = require("./model_aliases.cjs"); | ||
| const shortModel = reduceModelNameToIdentifier(attribution.model); | ||
| if (!attribution.routing) return shortModel; | ||
| if (!attribution.routing) { |
There was a problem hiding this comment.
[/codebase-design] Undocumented behavior change. Fallback-only runs (requested_model: auto, fallback_model: gpt-5.4, no routing) previously returned the short alias gpt54 through the removed line if (!attribution.routing) return shortModel;. They now return auto (powered by GPT 5.4). The PR body says fallback labels keep their behavior, and no test covers this path. If the new label is intended, state it in the PR body and add a test. If not, limit the new branch to observed evidence.
@copilot please address this.
There was a problem hiding this comment.
Fixed in e7842a3: fallback-only auto runs retain the prior model alias when there is no observed evidence; a regression test covers this behavior.
| if (requested.toLowerCase() !== "auto") return shortModel; | ||
| const models = new Set(); | ||
| if (phase === "agent") { | ||
| const sessionPath = require("path").join(require("path").dirname(infoPath), "usage/aw_session.jsonl"); |
There was a problem hiding this comment.
[/diagnosing-bugs] Possible silent miss. The comment at lines 243-244 says the agent post-step runs before usage/aw_session.jsonl is written. If the footer renders in that window, this lookup finds no events, and the label falls back to the single attribution.model, so multi-model output never appears. Confirm which step renders the footer. Then add a test with the session file absent, and one with agent records present through the footer path.
@copilot please address this.
There was a problem hiding this comment.
The footer is rendered through the safe-output message helpers. e7842a3 covers both absent session usage via persisted native-selection metadata and agent token-usage records through a rendered footer; the native model list is preserved when the session artifact is unavailable.
|
|
||
| describe("observed model attribution", () => { | ||
| it.each([ | ||
| ["gemini-3.8-flash", "auto (powered by Gemini 3.8 Flash)"], |
There was a problem hiding this comment.
[/tdd] Missing edge cases in this table: (1) variant spellings that collapse after formatting, such as google/gemini-3.8-flash with Gemini-3.8-Flash; (2) a trailing-slash identifier such as provider/; (3) fallback-only auto with no session file, which currently yields auto (powered by GPT 5.4) and is unasserted. Write these as failing tests before the fixes.
@copilot please address this.
There was a problem hiding this comment.
Fixed in e7842a3: added regression cases for formatted-name variants, empty provider-prefixed identifiers, and fallback-only auto labels.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
update.googleapis.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "update.googleapis.com"See Network Configuration for more information.
💥 [THE END] — Illustrated by Smoke Claude · claude · sonnet46 · ~89 AIC · ⌖ ~5 AIC · ⊞ 980
Comment /smoke-claude to run again
| } | ||
| const observedModels = attribution.selection?.status === "observed" ? attribution.selection.observed_models : null; | ||
| if (!models.size && Array.isArray(observedModels)) { | ||
| for (const value of observedModels) { |
There was a problem hiding this comment.
The fallback to observedModels from selection.observed_models when session usage is absent is a nice resilience improvement. Well handled!
There was a problem hiding this comment.
No change needed. I'm keeping this fallback. It now applies only when no fallback model is recorded (80fd66f).
There was a problem hiding this comment.
🟡 Changes recommended
Model attribution can publish incomplete or incorrect footers because proxy and runtime metadata are collected too late, native observations override fallback precedence, and retries discard earlier model history.
4 open issues
2 resolved since last review
Review effort: Balanced (auto)
| const offsets = new Map(); | ||
| /** @type {Map<string, string>} */ | ||
| const partial = new Map(); | ||
| const observedModels = new Set(); |
| if (!attribution.routing) return shortModel; | ||
| if (!attribution.routing) { | ||
| const requested = attribution.requestedModel || requestedModelIdentifier(env.GH_AW_INFO_MODEL || env.GH_AW_ENGINE_MODEL); | ||
| if (requested.toLowerCase() !== "auto") return shortModel; |
| const sessionPath = require("path").join(require("path").dirname(infoPath), "usage/aw_session.jsonl"); | ||
| for (const event of readModelRoutingSession(sessionPath)) { |
| const observedModels = attribution.selection?.status === "observed" ? attribution.selection.observed_models : null; | ||
| if (!models.size && Array.isArray(observedModels)) { |
|
Copilot assignment requested for the following outstanding work:
Once you start work, post this work summary as a PR comment without the leading Copilot mention, including the Sous-chef head, work, and state lines below, to record that the assignment is running. Apply the necessary fixes on this PR's existing branch, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved; stop when only human review or CI remains. Do not merge or trigger CI. Sous-chef requested-head: e7842a3
|
|
Copilot assignment requested for the following outstanding work:
Once you start work, post this work summary as a PR comment without the leading Copilot mention, including the Sous-chef head, work, and state lines below, to record that the assignment is running. Apply the necessary fixes on this PR's existing branch, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved; stop when only human review or CI remains. Do not merge or trigger CI. Sous-chef requested-head: e7842a3
|
|
Copilot assignment requested for the following outstanding work:
Once you start work, post this work summary as a PR comment without the leading Copilot mention, including the Sous-chef head, work, and state lines below, to record that the assignment is running. Apply the necessary fixes on this PR's existing branch, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved; stop when only human review or CI remains. Do not merge or trigger CI. Sous-chef requested-head: e7842a3
|
|
Copilot assignment requested for the following outstanding work:
Once you start work, post this work summary as a PR comment without the leading Copilot mention, including the Sous-chef head, work, and state lines below, to record that the assignment is running. Apply the necessary fixes on this PR's existing branch, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved; stop when only human review or CI remains. Do not merge or trigger CI. Sous-chef requested-head: e7842a3
|
|
Copilot assignment requested for the following outstanding work:
Once you start work, post this work summary as a PR comment without the leading Copilot mention, including the Sous-chef head, work, and state lines below, to record that the assignment is running. Apply the necessary fixes on this PR's existing branch, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved; stop when only human review or CI remains. Do not merge or trigger CI. Sous-chef requested-head: e7842a3
|
|
@copilot remove "Powered by" to save space, just a list of model names |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Copilot assignment requested for the following outstanding work:
Once you start work, post this work summary as a PR comment without the leading Copilot mention, including the Sous-chef head, work, and state lines below, to record that the assignment is running. Apply the necessary fixes on this PR's existing branch, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved; stop when only human review or CI remains. Do not merge or trigger CI. Sous-chef requested-head: d6f34ac
|
…red-by Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
I merged the latest Items 2–5 were already done in 80fd66f. This request will keep coming back until a maintainer resolves threads r4475080603, r4475080618, r4475165420 and r4475165477, which I can't resolve from here. My push won't start CI, so a maintainer also needs to re-run it. |
|
Copilot assignment requested for the following outstanding work:
Once you start work, post this work summary as a PR comment without the leading Copilot mention, including the Sous-chef head, work, and state lines below, to record that the assignment is running. Apply the necessary fixes on this PR's existing branch, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved; stop when only human review or CI remains. Do not merge or trigger CI. Sous-chef requested-head: a69a49d
|
No changes this time. The branch is already up to date with This request repeats because review threads r4475080603, r4475080618, r4475165420 and r4475165477 are still unresolved. A maintainer needs to resolve them, since I can't from here. CI also needs a maintainer to re-trigger it. |
|
Copilot assignment requested for the following outstanding work:
Once you start work, post this work summary as a PR comment without the leading Copilot mention, including the Sous-chef head, work, and state lines below, to record that the assignment is running. Apply the necessary fixes on this PR's existing branch, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved; stop when only human review or CI remains. Do not merge or trigger CI. Sous-chef requested-head: a69a49d
|
|
@copilot resolve the merge conflicts on this branch. |
…red-by # Conflicts: # actions/setup/js/copilot_model_selection.cjs Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Resolved in 2bf81a3. The only conflict was in |
|
Copilot assignment requested for the following outstanding work:
Once you start work, post this work summary as a PR comment without the leading Copilot mention, including the Sous-chef head, work, and state lines below, to record that the assignment is running. Apply the necessary fixes on this PR's existing branch, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved; stop when only human review or CI remains. Do not merge or trigger CI. Sous-chef requested-head: 2bf81a3
|
Starting work on the requested items. Sous-chef requested-head: 2bf81a3 |
…red-by Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
When a workflow requests
auto, attribution footers now include the observed model names, for examplecopilot · auto (powered by Gemini 3.8 Flash). Multiple observed agent models are deduplicated; classifier and detection models are excluded. Without concrete evidence, the label remainsauto. Existing routing and fallback labels retain their behavior.Validation:
make agent-report-progressandmake lint-cjspassed, including 157 impacted tests. Regression coverage includes default and custom footer templates, multiple models, and missing or invalid model evidence.✨ PR Review Safe Output Test - Run 41939432471
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
update.googleapis.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.