Tiered validation: fast gate for intermediate saves, tests only on final push#46927
Conversation
…y on final push - Add `agent-report-progress-no-test` Makefile target: same as `agent-report-progress` but skips test-unit (~30s vs 120s+) - Update AGENTS.md rule 2: use fast gate for intermediate report_progress calls; run full gate (with test-unit) only before the final push - Update developer SKILL.md: tiered checkpoint guide with build+fmt after each edit, no-test gate for intermediate saves, full gate for final push, plus timeout-budget guidance for test-unit Closes #46904 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🔍 PR Triage — Run §29791496271
Rationale: Tiered validation concept is sound but 263 files / 523k deletions is extremely risky. Needs careful scoped review and AGENTS.md alignment before undrafting.
|
|
@copilot merge main and recompile |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #46927 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
The changes are clean and well-structured. The tiered validation approach (agent-report-progress-no-test for intermediate saves, agent-report-progress for final push) is a sound optimization. AGENTS.md and SKILL.md are updated consistently. The smoke-call-workflow.lock.yml changes (secrets: inherit, reduced permissions, comment fix) look correct for a recompile artifact. No issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 14.2 AIC · ⌖ 4.34 AIC · ⊞ 5K
There was a problem hiding this comment.
Pull request overview
Introduces tiered validation to avoid repeated full test runs during intermediate saves.
Changes:
- Adds a fast, test-free intermediate validation target.
- Updates agent guidance for intermediate and final validation.
- Regenerates a smoke workflow lock file, introducing permission, context, and secret-handling regressions.
Show a summary per file
| File | Description |
|---|---|
Makefile |
Adds tiered validation targets and help text. |
AGENTS.md |
Defines intermediate and final validation rules. |
.github/skills/developer/SKILL.md |
Documents the tiered validation sequence. |
.github/workflows/smoke-call-workflow.lock.yml |
Regenerates reusable-workflow wiring. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 5
- Review effort level: Medium
| # Review the called workflow's frontmatter permissions in ./.github/workflows/smoke-workflow-call.md. | ||
| permissions: | ||
| actions: read | ||
| contents: read | ||
| issues: write | ||
| pull-requests: write | ||
| pull-requests: read |
| uses: ./.github/workflows/smoke-workflow-call.lock.yml | ||
| with: | ||
| aw_context: ${{ fromJSON(needs.safe_outputs.outputs.call_workflow_payload).aw_context }} | ||
| payload: ${{ needs.safe_outputs.outputs.call_workflow_payload }} |
| GH_AW_OTEL_GRAFANA_ENDPOINT: ${{ secrets.GH_AW_OTEL_GRAFANA_ENDPOINT }} | ||
| GH_AW_OTEL_SENTRY_AUTHORIZATION: ${{ secrets.GH_AW_OTEL_SENTRY_AUTHORIZATION }} | ||
| GH_AW_OTEL_SENTRY_ENDPOINT: ${{ secrets.GH_AW_OTEL_SENTRY_ENDPOINT }} | ||
| secrets: inherit |
| 4. **Before final handoff when time allows** | ||
| ```bash | ||
| make agent-finish |
|
|
||
| > **Key rule:** Run `test-unit` only before the **final** `report_progress` call, not before intermediate saves. Each unnecessary invocation adds 120+ seconds to total validation time. | ||
|
|
||
| > **Timeout budget:** `make test-unit` is expected to take up to 120 seconds. If it exceeds that, use `make test-impacted-go` to run only tests for packages affected by the current branch's changes. |
Done. Merged main and recompiled all 260 workflows (0 errors). |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /grill-with-docs — two issues flagged, no blocking defects.
📋 Key Themes & Highlights
Key Themes
secrets: inheritsecurity trade-off (smoke-call-workflow.lock.yml): switching from 6 explicit secrets tosecrets: inheritbroadens credential exposure. Needs confirmation this is intentional and ideally a comment explaining why.- "intermediate" is undefined (AGENTS.md): the optimisation only works if agents can reliably identify the final
report_progresscall. Without a clear heuristic, agents may either skip tests on the last push or run full tests on every save — exactly the waste this PR aims to prevent.
Positive Highlights
- ✅ The tiered validation idea is sound and well-motivated — removing redundant 120s test runs is a real win.
- ✅ The
Makefiletarget names and comments are clear and self-documenting. - ✅ SKILL.md callout blocks improve signal-to-noise for agents scanning the doc.
- ✅ Permission reduction in the lock file is a good security improvement where intentional.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 26.2 AIC · ⌖ 4.61 AIC · ⊞ 6.7K
Comment /matt to run again
| GH_AW_OTEL_GRAFANA_ENDPOINT: ${{ secrets.GH_AW_OTEL_GRAFANA_ENDPOINT }} | ||
| GH_AW_OTEL_SENTRY_AUTHORIZATION: ${{ secrets.GH_AW_OTEL_SENTRY_AUTHORIZATION }} | ||
| GH_AW_OTEL_SENTRY_ENDPOINT: ${{ secrets.GH_AW_OTEL_SENTRY_ENDPOINT }} | ||
| secrets: inherit |
There was a problem hiding this comment.
[/codebase-design] Switching to secrets: inherit expands the secret exposure surface from the 6 explicitly-named secrets to all secrets available in the caller context.
💡 Why this matters
The previous explicit list constrained which credentials the called workflow could access. secrets: inherit forwards every secret the caller has, including any future secrets added to the repo. If smoke-workflow-call does not need all those secrets, this violates the principle of least privilege.
Consider whether the reduction to secrets: inherit was intentional or an artefact of a make recompile pass. If intentional, document why in the PR description. If not intentional, restore the explicit list.
@copilot please address this.
| @@ -22,7 +22,8 @@ Everything else should be loaded **lazily** through skills only when needed. | |||
| ## Critical Rules (Always Applicable) | |||
|
|
|||
| 1. If you changed files, use `report_progress` to commit and push. | |||
There was a problem hiding this comment.
[/grill-with-docs] The term "intermediate" is not defined — agents must infer what counts as an intermediate vs. final report_progress call, which could lead to skipping tests on what is actually the final push.
💡 Suggested clarification
Add a concrete heuristic, e.g.:
Intermediate = any save before the PR is ready for human review. Final = the save that creates or updates the PR targeting merge.
Without this, an agent unsure whether its current save is the last one may default to agent-report-progress every time, eliminating the optimisation.
@copilot please address this.
There was a problem hiding this comment.
Two blocking issues in the workflow change
The Makefile and documentation changes (tiered validation, agent-report-progress-no-test) are well-structured. Two issues in smoke-call-workflow.lock.yml need resolution before merge.
🔍 Findings
[High] Permission downgrade vs. called workflow requirements — the caller now grants only contents: read + pull-requests: read. GitHub requires the caller to grant at least every permission the callee declares. If smoke-workflow-call.lock.yml job blocks still declare pull-requests: write or issues: write, every invocation will fail at runtime with a permissions error. Needs explicit verification that the callee's permissions were reduced to match.
[Medium] secrets: inherit broadens secret exposure — replacing the explicit secrets list passes all repository secrets to the called workflow unconditionally. Unless the prior explicit list was incomplete by design, prefer an explicit list to maintain least-privilege.
🔎 Code quality review by PR Code Quality Reviewer · 40.8 AIC · ⌖ 4.61 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
.github/workflows/smoke-call-workflow.lock.yml:1118
Permission downgrade may break the called workflow at runtime: the caller now grants only contents: read + pull-requests: read, but the called workflow likely still declares pull-requests: write (and possibly issues: write / actions: read). GitHub requires the caller to grant at least the permissions the callee declares; granting fewer causes a hard runtime error.
<details>
<summary>💡 What to verify</summary>
Check smoke-workflow-call.lock.yml job-level permissions: blocks.…
.github/workflows/smoke-call-workflow.lock.yml:1127
secrets: inherit exposes all repository secrets to the called workflow: the previous explicit list forwarded only the specific secrets the workflow needed. secrets: inherit passes every secret in the repo (API keys, signing keys, tokens) to the called workflow unconditionally.
<details>
<summary>💡 Why this matters</summary>
With explicit secrets forwarding, a compromised or mis-invoked called workflow could only access a bounded set of secrets. With secrets: inherit, the blast radi…
|
🎉 This pull request is included in a new release. Release: |
make agent-report-progressincludestest-unit(120–233s) and was run before everyreport_progresscall per AGENTS.md rule 2. Multiple saves per session meant redundant full test runs, wasting hundreds of seconds late in the session where failures are costliest.Changes
Makefileagent-report-progress-no-testtarget: build + fmt + lint + workflow-drift, notest-unit(~30s)agent-report-progressretainstest-unit; updated comment to mark it as the final-push gateAGENTS.md— rule 2 split into two:make agent-report-progress-no-testmake agent-report-progress(withtest-unit, once).github/skills/developer/SKILL.md— validation checkpoints rewritten as a tiered sequence:Adds callouts for the key rule (run
test-unitonce, before the final push only) and a timeout-budget note pointing tomake test-impacted-goas a scoped fallback whentest-unitruns long.