-
Notifications
You must be signed in to change notification settings - Fork 32
fix: drift agent exhaustive release coverage #765
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -683,11 +683,16 @@ foreach ($manifestPath in $manifests) { | |
| $untrackedCount = ($closedResult.untracked | Measure-Object).Count | ||
| if ($untrackedCount -gt 0) { | ||
| Write-Host " ⚠️ Found $untrackedCount recently closed issue(s) not in manifest:" -ForegroundColor Yellow | ||
| foreach ($issue in $closedResult.untracked) { | ||
| foreach ($issue in ($closedResult.untracked | Select-Object -First 10)) { | ||
| $labels = if ($issue.labels) { " [$($issue.labels -join ', ')]" } else { '' } | ||
| Write-Host " #$($issue.number): $($issue.title)$labels" -ForegroundColor Yellow | ||
| } | ||
| $results[-1].untracked_closed_issues = @($closedResult.untracked) | ||
| if ($untrackedCount -gt 10) { | ||
| Write-Host " ... and $($untrackedCount - 10) more" -ForegroundColor Yellow | ||
| } | ||
| # Cap at 20 in JSON to avoid bloating the report | ||
| $results[-1].untracked_closed_issues = @($closedResult.untracked | Select-Object -First 20) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 MODERATE — JSON cap at 20 may hide important untracked issues (Flagged by: 2/3 reviewers) If a security-relevant closed issue ranks beyond position 20 in the API response order, it's silently dropped from the JSON artifact. The agent has no way to retrieve it — The new Fix: Consider keeping the full list in the JSON (the size is manageable — 20 issues ≈ a few KB) and only capping the console display, or provide a secondary artifact file with the complete list. |
||
| $results[-1].untracked_closed_issues_total = $untrackedCount | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟢 MINOR — This new field is a useful addition, but it has two gaps:
Fix: Add extraction of this total to the agent's summary snippet, and consider setting it to |
||
| } | ||
| else { | ||
| Write-Host " ✅ No new closed issues (checked $($closedResult.total_checked) in last 90 days)" -ForegroundColor Green | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -81,14 +81,25 @@ timeout-minutes: 30 | |
| The staleness check ran in `steps:` (before you started) with authenticated `gh` CLI. | ||
| **Do NOT run Check-Staleness.ps1 yourself** — `gh` CLI is not authenticated inside this container. | ||
|
|
||
| Read the results files: | ||
| Read the results. The file is large — extract only the releases and key signals: | ||
| ```bash | ||
| cat /tmp/drift-results/staleness.json | ||
| # Check if changes were detected | ||
| python3 -c "import json; d=json.load(open('/tmp/drift-results/staleness.json')); print('changes_detected:', d.get('changes_detected')); [print(f' {s[\"type\"]}: {s.get(\"last_reviewed_release\",\"\")} -> releases={len(s.get(\"result\",{}).get(\"releases\",[]))}') for m in d['manifests'] for s in m['sources'] if s['type']=='releases']" | ||
| ``` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 MODERATE — Both Python extraction snippets (this line and line ~96) use Note: one reviewer argued a loud traceback is better than silent empty results, since the agent can investigate. This is a valid trade-off — if you keep hard access, consider adding an explicit error-check block before extraction: if 'manifests' not in d:
print(f'ERROR: no manifests key. Keys: {list(d.keys())}') |
||
|
|
||
| If the file contains `"changes_detected": true`, also read: | ||
| If changes detected, extract the release notes: | ||
| ```bash | ||
| cat /tmp/drift-results/upstream.json | ||
| python3 -c " | ||
| import json | ||
|
Comment on lines
91
to
+93
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 MODERATE — Orphaned upstream scan step (Flagged by: 3/3 reviewers) The old instruction Commit-level change data captured in Fix: Either (a) remove the upstream scan step entirely since its output is dead, or (b) add a Python extraction snippet for |
||
| d=json.load(open('/tmp/drift-results/staleness.json')) | ||
| for m in d['manifests']: | ||
| for s in m['sources']: | ||
| if s['type']=='releases' and s.get('result',{}).get('releases'): | ||
| for r in s['result']['releases']: | ||
| print(f'=== {r[\"tag\"]} ({r[\"published_at\"]}) ===') | ||
| print(r.get('release_notes','')[:8000]) | ||
| print() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 MODERATE — 8000-char truncation silently drops release note tail (Flagged by: 2/3 reviewers) The Fix: Add a truncation marker so the agent knows content was lost: notes = r.get('release_notes', '')
print(notes[:8000])
if len(notes) > 8000:
print(f'... TRUNCATED ({len(notes)} total chars, {len(notes) - 8000} chars omitted)')Also consider raising the limit (e.g., 50000 chars) or removing it entirely if token budget allows. |
||
| " | ||
| ``` | ||
|
|
||
| If there were errors, check: | ||
|
|
@@ -112,27 +123,68 @@ Do not create issues, PRs, or comments. Stop after calling noop. | |
|
|
||
| ## Analyze and Update | ||
|
|
||
| Read the staleness report above. For each signal, read the affected skill files and make updates: | ||
| - `.claude/skills/gh-aw-guide/SKILL.md` | ||
| - `.claude/skills/gh-aw-guide/references/architecture.md` | ||
| - `.github/instructions/gh-aw-workflows.instructions.md` | ||
| - `.github/instructions/gh-aw-workflows.sync.yaml` | ||
| ### Step 2: Build a checklist of ALL changes from release notes | ||
|
|
||
| Before editing any files, read EVERY release note above and build a complete list of changes. For each item, classify: | ||
| - **P0** = factually wrong in our docs | ||
| - **P1** = security-relevant change | ||
| - **P2** = new feature, new safe output, new config option, behavior change, bug fix that affects workflow authors | ||
| - **P3** = internal/cosmetic only (OTLP traces, CI pipeline changes, internal refactors) | ||
|
|
||
| **You MUST implement P0, P1, AND P2 items. Only skip P3.** | ||
|
|
||
| ### Step 3: Read current skill files | ||
|
|
||
| Read ALL of these to understand what we currently cover: | ||
| ```bash | ||
| cat .claude/skills/gh-aw-guide/SKILL.md | ||
| cat .claude/skills/gh-aw-guide/references/architecture.md | ||
| cat .github/instructions/gh-aw-workflows.instructions.md | ||
| cat .github/instructions/gh-aw-workflows.sync.yaml | ||
| ``` | ||
|
|
||
| ### Step 4: Implement ALL changes | ||
|
|
||
| For EACH P0/P1/P2 item from your checklist: | ||
| 1. Find the right section in the skill files | ||
| 2. Add or update the content | ||
| 3. Check off the item | ||
|
|
||
| **Where to add each type of change:** | ||
| - New safe output → anti-patterns table in SKILL.md + Safe Outputs section | ||
| - New frontmatter option → Frontmatter Features section in SKILL.md | ||
| - New trigger behavior → Trigger Selection Guide in SKILL.md | ||
| - Security change → Security-Critical Patterns in SKILL.md | ||
| - Protected files change → architecture.md Protected Files section | ||
| - Bug fix affecting workflow authors → relevant section + Known Issues if applicable | ||
|
|
||
| ### Step 5: Update the sync manifest | ||
|
|
||
| In `gh-aw-workflows.sync.yaml`, update `last_reviewed_release` to the latest tag. | ||
|
|
||
| ### Step 6: Verify completeness | ||
|
|
||
| Before committing, re-read your checklist. Every P0/P1/P2 item must be addressed. If any are missing, go back and add them. | ||
|
|
||
| ### Rules | ||
| 1. **Respect `divergence` sections** — NEVER remove: "Security Boundaries", "Safe Pattern: Checkout + Restore", "Common Patterns" | ||
| 2. **Classify P0-P3:** P0=factually wrong, P1=security, P2=new features, P3=nice-to-have | ||
| 3. **Auto-fix P0, P1, AND P2.** Only skip P3 (cosmetic/nice-to-have). | ||
| 4. **For new releases:** Read ALL release notes in the report. For each new feature, check if our skill covers it. Add new features to the anti-patterns table, trigger guide, or security patterns as appropriate. Update the `last_reviewed_release` field in `gh-aw-workflows.sync.yaml` to the latest tag after processing. | ||
| 5. **Update sync manifest** — `resolution_expected`, `last_reviewed_release`, new issues | ||
| 2. **Be exhaustive** — every author-facing feature, behavior change, and bug fix must be documented | ||
| 3. **Include YAML examples** for new config options | ||
| 4. **List what was skipped as P3** in the PR description with reasoning | ||
|
|
||
| Commit each change: | ||
| Commit all changes together: | ||
| ```bash | ||
| git add <specific-files> | ||
| git commit -m "docs: update <what> | ||
| git add .claude/skills/gh-aw-guide/SKILL.md .claude/skills/gh-aw-guide/references/architecture.md .github/instructions/gh-aw-workflows.instructions.md .github/instructions/gh-aw-workflows.sync.yaml | ||
| git commit -m "docs: update gh-aw skill for <versions> | ||
|
|
||
| Co-authored-by: copilot-agentic-workflow[bot] <224017+copilot-agentic-workflow[bot]@users.noreply.github.com>" | ||
| ``` | ||
|
|
||
| ## Create PR | ||
|
|
||
| The `create-pull-request` safe output packages changes into a draft PR. | ||
|
|
||
| In the PR body, include: | ||
| 1. **Complete list of P0/P1/P2 items** with which file/section was updated | ||
| 2. **Complete list of P3 items skipped** with reasoning | ||
| 3. The release versions covered | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 MODERATE — Magic number caps are fragile (Flagged by: 3/3 reviewers)
The literal
10appears three times (Select-Object -First 10,-gt 10,$untrackedCount - 10) and must stay in sync. Similarly20for the JSON cap. If a maintainer changes one without the others, the "... and X more" message will silently produce incorrect arithmetic.Fix: Extract named constants at the top of the block:
Then reference
$displayCapand$jsonCapthroughout.