runtime: reject unresolved GitHub Actions expressions in model name#46712
Conversation
…name Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot ensure that expressions with experiments... are properly expanded for the model field. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Experiment model variants work correctly with this check. When frontmatter uses |
|
@copilot merge main and recompile |
…heck-model-name Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done — merged main and recompiled all 258 workflows (0 errors, lock files in sync). |
|
Thanks for working on this runtime validation fix! The code quality is strong — the logic is clear, tests are comprehensive, and the problem it solves (unresolved GitHub Actions expressions silently poisoning aw_info.json) is a real pain point. However, this PR requires a process correction. Per CONTRIBUTING.md, Traditional Pull Requests Are Not Enabled for non-Core team members. The proper workflow is:
This PR should have started as an issue-based agentic plan rather than a direct PR. Since a core team member (like Once the process is aligned, this change is ready to merge. The validation logic is solid, and the test coverage (two failure cases + two pass cases) is appropriate. If you'd like to contribute via the proper channel, you can assign this prompt to your agent:
|
|
/smoke-copilot |
|
📰 BREAKING: Smoke Copilot is now investigating this issue comment. Sources say the story is developing... |
There was a problem hiding this comment.
Pull request overview
Adds runtime validation to prevent unresolved GitHub Actions expressions from becoming model metadata.
Changes:
- Rejects unresolved model expressions with
ERR_CONFIG. - Adds failure and success tests.
- Includes an unrelated generated JSON reformat.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/generate_aw_info.cjs |
Validates model metadata before writing run information. |
actions/setup/js/generate_aw_info.test.cjs |
Tests rejected and accepted model values. |
pkg/agentdrain/data/default_weights.json |
Reformats generated weights without semantic changes. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Medium
| "span_id", | ||
| "timestamp" | ||
| ], | ||
| "ExcludeFields": ["session_id", "trace_id", "span_id", "timestamp"], |
There was a problem hiding this comment.
Reverted pkg/agentdrain/data/default_weights.json in 7b76783, so this PR no longer carries the unrelated formatting rewrite.
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.
Cave smoke done. One stray file seen.
📰 BREAKING: Report filed by Smoke Copilot · 117.8 AIC · ⌖ 8.24 AIC · ⊞ 19K
Comment /smoke-copilot to run again
Add label smoke to run again
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #46712 does not have the 'implementation' label and has only 8 new lines of code in business logic directories (threshold: 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — approving with no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Fail-fast validation: The
$\{\{regex precisely targets the GH Actions expression opening delimiter — correct and minimal. - Test coverage: Five tests cover both failure paths (plain expression, expression-with-fallback) and three pass paths (plain string, empty, resolved variant). Good use of Arrange/Act/Assert throughout.
- Unrelated change: The
default_weights.jsonreformatting is already called out in an existing review comment — no further action needed here.
Positive Highlights
- ✅ Clean separation: validation fires before
awInfois built, preventing poisoned writes. - ✅ Error message includes the offending value, making debugging easy for users.
- ✅
ERR_CONFIGprefix is consistent with the project's error code conventions.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 15.2 AIC · ⌖ 4.39 AIC · ⊞ 6.7K
Comment /matt to run again
There was a problem hiding this comment.
The validation logic is correct and well-tested. Checking for $\{\{ as the sentinel for an unresolved expression is the right approach — GitHub Actions never emits that substring in a fully-evaluated value.
The five new test cases cover the important scenarios (bare expression, expression-with-fallback, plain string, empty string, resolved variant). The PR description says four tests but there are actually five — minor discrepancy.
The unrelated default_weights.json reformatting already has an open review comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 13.3 AIC · ⌖ 4.97 AIC · ⊞ 5K
🧪 Test Quality Sentinel Report❌ Test Quality Score: 62/100 — Acceptable (but fails quality threshold)
📊 Metrics (5 tests)
|
There was a problem hiding this comment.
Non-blocking review. One low-severity concern found (see inline comment on line 56). The guard logic, tests, and JSON reformatting are all correct.
🔎 Code quality review by PR Code Quality Reviewer · 26.2 AIC · ⌖ 4.38 AIC · ⊞ 5.6K
Comment /review to run again
| } | ||
|
|
||
| // Build awInfo from env vars (compile-time) + context (runtime) | ||
| const model = process.env.GH_AW_INFO_MODEL || ""; |
There was a problem hiding this comment.
Raw model value interpolated into error message — risk of log injection/corruption: if the model string contains ANSI escape sequences or newlines, it can corrupt GitHub Actions log output or mislead log parsers.
💡 Suggested fix
Stringify the value before embedding it in the message:
const message = `${ERR_CONFIG}: GH_AW_INFO_MODEL contains an unresolved GitHub Actions expression: ${JSON.stringify(model)}`;This ensures the full raw value is safely visible in logs without escaping into the surrounding log stream.
There was a problem hiding this comment.
Updated the failure path in 7b76783 to stringify GH_AW_INFO_MODEL before embedding it in the error message, and kept the assertion coverage aligned with that output.
|
@copilot run pr-finisher skip |
🤖 PR Triage - run 29724276600
Score breakdown: Impact 35/50 - Urgency 22/30 - Quality 13/20 Rationale: Silent bug: unresolved GitHub Actions vars poison model name at runtime. CI passing (canary-go, Windows). Tests included. New PR. Next: Human review + merge if CI clean.
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot please run the Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "pi.dev"See Network Configuration for more information.
|
|
🎉 This pull request is included in a new release. Release: |
When a
vars.*expression is used formodelin workflow frontmatter but the variable isn't defined, GitHub Actions passes the literal${{ vars.MODEL }}string at runtime — silently poisoningaw_info.jsonwith a non-functional model name.Changes
generate_aw_info.cjs: Before buildingawInfo, validates thatGH_AW_INFO_MODELdoes not contain${{. Fails fast viacore.setFailed+ throw with anERR_CONFIG-prefixed message.generate_aw_info.test.cjs: Four new tests — two failure cases (${{ vars.COPILOT_MODEL }}, expression-with-fallback syntax) and two pass cases (plain model string, empty string).