[EMB-359] zeus agent review trigger#731
Conversation
🔍 QA Review — EMB-359 — PR #731
🧠 What this PR doesAdds 📋 Acceptance Criteria Check
🔁 Resolution Status — All Previously Flagged Items
All 3 previously flagged items are now resolved. No new issues found in the post-review commit. 🔍 Post-Review Commit Analysis (
|
| Check | Result |
|---|---|
if: condition uses explicit ${{ }} syntax |
✅ |
| Action pinned to 40-char commit SHA | ✅ fee1f7d63c2ff003460e3d139729b119787bc349 |
Token scoped to repositories: QA only |
✅ |
Job permissions minimal (contents: read) |
✅ |
event_type matches dispatch workflow (qa-review-embeddables) |
✅ |
client_payload.pr_url field matches expected payload structure |
✅ |
jq and gh available on ubuntu-latest |
✅ Pre-installed |
| PR body documents intentional spec deviations | ✅ |
🧪 Test Coverage
| Layer | Status | Notes |
|---|---|---|
| Unit (Vitest) | N/A | Workflow-only PR — no TypeScript changed |
| E2E (Playwright) | N/A | No UI flow changed |
| Integration smoke | ✅ Live evidence | Label trigger → dispatch → Zeus review comment confirmed end-to-end by this very review |
🔗 Downstream Impact
Fully compatible with qa-review-embeddables-dispatch.yml in lifinance/QA (QA-60 ✅). Payload structure (event_type: qa-review-embeddables, client_payload.pr_url) matches exactly. Token scope narrows to lifinance/QA only — no impact on other repos.
🤖 Zeus QA Agent — 2026-05-18 | Ticket coverage: Full
There was a problem hiding this comment.
Requesting changes on 3 items — each requires either a code fix or an explicit acceptance comment with justification before this review is considered complete.
| # | Severity | Type | Issue / File |
|---|---|---|---|
| 1 | 🟠 High | Config | Secret names diverge from AC-2 — org admin will provision wrong credentials |
| 2 | 🟡 Medium | Code | .github/workflows/qa-review-trigger.yml — if: condition uses bare string instead of ${{ }} expression |
| 3 | 🟡 Medium | Config | PR body is empty — intentional spec deviations are undocumented |
1. [High] Secret names diverge from AC-2 — org admin will provision wrong credentials
The ticket's AC-2 and Prerequisites section specify secrets.APP_PRIVATE_KEY and vars.APP_ID, but the workflow reads secrets.QA_APP_PRIVATE_KEY and secrets.QA_APP_ID. An org admin following the ticket will provision the wrong names, causing the Create GitHub App token step to fail silently. Either update Linear AC-2 to match the workflow's names (secrets.QA_APP_ID + secrets.QA_APP_PRIVATE_KEY, both as secrets), or align the workflow back to the ticket spec. Add a note to the PR description stating which names the org admin must provision.
2. [Medium] if: condition — use explicit ${{ }} expression syntax
Change line 9 from:
if: github.event.label.name == 'Agent review'
to:
if: ${{ github.event.label.name == 'Agent review' }}
The bare-string form works today but is ambiguous to readers and fragile if the label name ever includes YAML-special characters.
3. [Medium] PR body is empty — document intentional spec deviations
The PR silently changes vars.APP_ID → secrets.QA_APP_ID and replaces peter-evans/repository-dispatch with gh api. Both are defensible, but with an empty body, reviewers and the provisioning admin have no way to know these are intentional. Fill in the PR description explaining the rationale for each deviation.
💡 Once you've addressed the items above, re-apply the "Agent review" label to trigger an automated re-review.
|
@lifi-qa-agent — addressing the open medium/high items we're intentionally not changing: Issue 1 (High) — Secret names diverge from AC-2 AC-1 — Label not verifiable from code AC-4 — End-to-end not verifiable statically Test coverage — Integration smoke test missing |
There was a problem hiding this comment.
Requesting changes on 1 item — it requires either a code fix or an explicit acceptance comment with justification before this review is considered complete.
| # | Severity | Type | Issue / File |
|---|---|---|---|
| 1 | 🟡 Medium | Code | .github/workflows/qa-review-trigger.yml — if: condition uses bare string without explicit ${{ }} expression |
1. [Medium] if: condition — use explicit ${{ }} expression syntax
.github/workflows/qa-review-trigger.yml line 6 still reads:
if: github.event.label.name == 'Agent review'This item was flagged in the first review and has neither been fixed in code nor addressed in the developer response (Issues 1 and 3 were addressed, but Issue 2 was not mentioned). The recommended form is:
if: ${{ github.event.label.name == 'Agent review' }}The bare-string form works correctly in GitHub Actions today, but the explicit ${{ }} wrapper is recommended by GitHub's security hardening guidelines and aligns with the convention used in other workflow files in this repo. This is a one-line change. Alternatively, leave an explicit comment accepting the bare-string form with justification — either resolves this item.
💡 Once you've addressed the items above, re-apply the "Agent review" label to trigger an automated re-review.
…rrow token scope
- Wrap `if:` condition in `${{ }}` for explicit expression evaluation
- Add `repositories: QA` to create-github-app-token to restrict the token
to the dispatch target only, reducing blast radius of a compromised token
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
All 3 previously flagged items resolved. Issue 2 (if: bare string) fixed in commit 3c282fd with explicit ${{ }} syntax; bonus token scope narrowing added. Workflow is clean, SHA-pinned, and live end-to-end. ✅ QA Pass.
| on: | ||
| pull_request: | ||
| types: [labeled] | ||
|
|
There was a problem hiding this comment.
Suggestion:
concurrency:
group: qa-review-${{ github.event.pull_request.number }}
cancel-in-progress: true
We can add it to handle cases if someone removes and re-applies the label quickly, and two dispatch runs fire. So a concurrency group with cancel-in-progress: true would deduplicate.
There was a problem hiding this comment.
good idea. addressed
| jobs: | ||
| trigger-zeus: | ||
| if: ${{ github.event.label.name == 'Agent review' }} | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
timeout-minutes: 5
It will be useful if anything gets stuck, default 6 hours timeout might be long.
There was a problem hiding this comment.
true, this can happen. Added the timeout! thank you
Which Linear task is linked to this PR?
EMB-359 — feat: add "Agent review" label and Zeus QA trigger workflow
Why was it implemented this way?
This PR adds
.github/workflows/qa-review-trigger.yml, which listens for thelabeledevent and fires arepository_dispatchtolifinance/QAwhen the "Agent review" label is applied to a PR. The receiving workflow (qa-review-embeddables-dispatch.yml, merged under QA-60) then runs Zeus and posts a structured review comment.Two intentional deviations from the ticket spec:
Secret names:
QA_APP_ID/QA_APP_PRIVATE_KEYinstead ofAPP_ID/APP_PRIVATE_KEYThe ticket originally proposed generic names (
APP_ID,APP_PRIVATE_KEY). These were renamed with theQA_prefix because the credentials are exclusively used by the lifi-qa-agent and the prefix makes their purpose unambiguous to anyone browsing the repo's secrets list. The org admin must provisionsecrets.QA_APP_IDandsecrets.QA_APP_PRIVATE_KEYinlifinance/widget.gh apiinstead ofpeter-evans/repository-dispatchReplaces a third-party action with a native
gh apiCLI call. This avoids an additional external action dependency while achieving the same result with the same SHA-pinned security posture.Checklist before requesting a review