updated github action for model changes#2564
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Removes the date-based lookback filter from the model-sync GitHub Action, replacing it with ID-pattern heuristics so the detection script considers all gateway models regardless of release date.
|
There was a problem hiding this comment.
Low urgency. The shift from date-based to keyword-based filtering is a reasonable simplification. Three suggestions below — the most impactful is moving more skip-logic into code rather than relying entirely on the Claude prompt to filter models that are explicitly listed under "New models to add."
| const rawId = m.id.slice(slashIndex + 1); | ||
| if (rawId.endsWith('-chat')) return false; | ||
| if (rawId.includes('-oss-') || rawId.startsWith('oss-')) return false; |
There was a problem hiding this comment.
The code-level filters only cover -chat and oss, but the prompt's SKIP rules cover a much broader set (instruct, embedding, tts, dall-e, realtime, audio, -image, dated snapshots, etc.). This means models matching those keywords still pass through to newModels and appear in the prompt's "New models to add" section — then Claude is asked to ignore them.
Consider filtering more of these at the code level (at minimum -image/-image-preview suffixes and instruct/embedding keywords). This reduces noise in the prompt and avoids relying on Claude to correctly skip models that are explicitly presented to it as new.
| --- | ||
|
|
||
| Add new models: ${modelSummary} | ||
| Add new models: <comma-separated list of provider/model-id for each model you added> |
There was a problem hiding this comment.
The changeset description is now a literal template Add new models: <comma-separated list ...> that Claude fills in. Since Claude is also told to skip certain models from the "New models to add" list, the generated description must exclude skipped models. If Claude misreads this and includes the angle-bracket placeholder literally, or lists all detected models including skipped ones, the changeset will be wrong.
Consider generating the description dynamically in code (as modelSummary previously did) and instructing Claude to adjust it only if it skips models, rather than generating it from scratch.
| const modelList = newModels.map((m) => `- ${m.provider}/${m.id}`).join('\n'); | ||
| const modelSummary = newModels.map((m) => `${m.provider}/${m.id}`).join(', '); | ||
| const today = new Date().toISOString().split('T')[0]; | ||
| const slug = Math.random().toString(36).slice(2, 8); |
There was a problem hiding this comment.
Nit: Math.random().toString(36).slice(2, 8) is fine for collision avoidance, but since the workflow's concurrency: { group: model-sync, cancel-in-progress: false } guarantees only one run at a time and each run already includes today's date — collisions can't actually happen. The slug is harmless but a brief comment explaining why it's there (future-proofing if concurrency changes) would help maintainers.
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This PR simplifies the model-sync GitHub Action by removing date-based filtering (DAYS parameter and released timestamp checks) in favor of ID-pattern-based filtering. The changes are well-scoped to internal CI/CD tooling with no customer-facing impact.
🔴❗ Critical (0) ❗🔴
None.
🟠⚠️ Major (0) 🟠⚠️
None.
🟡 Minor (0) 🟡
None.
💭 Consider (1) 💭
💭 1) model-sync.yml Reduced manual override flexibility
Issue: The days workflow_dispatch input has been removed, so operators can no longer manually trigger with a custom lookback window (e.g., DAYS=90 for backfills after outages).
Why: This is a reasonable architectural simplification since the new approach compares against the full gateway model list. If backfill capability is ever needed, a different mechanism would be required.
Refs: model-sync.yml:6
✅ APPROVE
Summary: Clean PR that simplifies the model detection logic. The code is correct, the filtering approach is sound, and the changeset slug addition prevents filename collisions. The security posture (SHA-pinned actions, minimal permissions) remains strong. Good to merge! 🚀
Discarded (4)
| Location | Issue | Reason Discarded |
|---|---|---|
detect-model-changes.ts:168 |
Changeset filename collision prevention | Positive observation — not an issue |
detect-model-changes.ts:69-72 |
ID-pattern filtering replaces date-based filtering | Informational observation — intentional design |
model-sync.yml |
Actions properly pinned to SHAs | Positive observation — not an issue |
model-sync.yml:16-18 |
Workflow permissions follow least-privilege | Positive observation — not an issue |
Reviewers (2)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-devops |
5 | 0 | 1 | 0 | 0 | 0 | 4 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 5 | 0 | 1 | 0 | 0 | 0 | 4 |
Ito Test Report ❌12 test cases ran. 11 passed, 1 failed. ✅ Most verification checks for workflow configuration, model filtering behavior, idempotency safeguards, and unauthorized-access boundaries behaved as expected. 🔍 One security-relevant defect was confirmed: prompt content is built from untrusted model IDs without escaping, allowing injected markdown/control text to break prompt structure. ✅ Passed (11)
❌ Failed (1)Prompt injection resilience from model IDs – Failed
New models to add: |
No description provided.