fix: drift check now detects new releases and auto-updates skill#763
fix: drift check now detects new releases and auto-updates skill#763
Conversation
Three bugs fixed in the drift detection workflow: 1. changes_detected never included new releases — the script fetched the latest release but never flagged it as a change. Added hasNewReleases to the detection logic. 2. Only fetched 1 release (per_page=1) — missed intermediate releases. New Get-GitHubReleasesSince fetches up to 10 and compares against last_reviewed_release watermark in the sync manifest. 3. Release notes truncated at 2000 chars — increased to 5000 to capture full feature lists from major releases like v0.70.0. Also: changed agent rules from 'only auto-fix P0/P1, note P2' to 'auto-fix P0/P1/P2' so new features are actually added to the skill, not just mentioned in the PR description. Tested locally: correctly detects v0.70.0 and v0.71.0 as new since last_reviewed_release v0.69.3. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Drift Detection Review — PR #763
Overall the changes are well-structured and address real gaps (releases never triggered changes_detected, only 1 release fetched). The manifest parser extension and the $hasNewReleases integration are clean.
Findings
| # | Severity | File | Line | Summary |
|---|---|---|---|---|
| 1 | 🟡 MODERATE | Check-Staleness.ps1 |
156–172 | Get-GitHubReleasesSince silently drops releases when $SinceTag isn't found in the top 10 — the agent will skip intermediate release notes permanently |
| 2 | 🟢 MINOR | Check-Staleness.ps1 |
131 | Pre-existing: $release.body.Length assumes non-null body (safe in pwsh 7, not pwsh 5) |
Finding #1 is the only actionable item — adding a watermark_found flag to the return value would let the agent (or a future iteration of the script) paginate or warn when the watermark falls outside the fetch window.
Generated by Expert Code Review (auto) for issue #763 · ● 7M
| # Truncate release notes to first 2000 chars to keep report manageable | ||
| $body = if ($release.body.Length -gt 2000) { $release.body.Substring(0, 2000) + '...' } else { $release.body } | ||
| # Truncate release notes to first 5000 chars | ||
| $body = if ($release.body.Length -gt 5000) { $release.body.Substring(0, 5000) + '...' } else { $release.body } |
There was a problem hiding this comment.
🟢 MINOR — Pre-existing null-safety note (not a regression)
Both Get-GitHubLatestRelease (here) and the new Get-GitHubReleasesSince (line 173) access $release.body.Length / $r.body.Length without a null guard. This works in PowerShell 7 ($null.Length → 0, so the else branch returns $null — no crash), but would fail in PowerShell 5.
Since the workflow invokes pwsh (PowerShell 7), this is safe today. Just noting for awareness — if portability matters, a guard like if ($release.body -and $release.body.Length -gt 5000) would be more robust.
| # Fetch recent releases (up to 10) | ||
| $json = gh api "repos/$Repo/releases?per_page=10" --jq '[.[] | {tag_name: .tag_name, name: .name, published_at: .published_at, body: .body}]' 2>&1 | ||
| if ($LASTEXITCODE -ne 0) { | ||
| return @{ | ||
| status = 'error' | ||
| repo = $Repo | ||
| error = "Failed to fetch releases: $json" | ||
| } | ||
| } | ||
| if (-not $json -or $json -eq 'null' -or $json -eq '[]') { | ||
| return @{ status = 'ok'; repo = $Repo; releases = @() } | ||
| } | ||
| $allReleases = $json | ConvertFrom-Json | ||
| # Collect releases newer than $SinceTag | ||
| $newReleases = @() | ||
| foreach ($r in $allReleases) { | ||
| if ($r.tag_name -eq $SinceTag) { break } |
There was a problem hiding this comment.
🟡 MODERATE — Silent data loss when watermark tag is not in the top 10 releases
per_page=10 fetches at most 10 releases. The loop breaks when it finds $SinceTag, but if the watermark is older than the 10th release (e.g., drift check was disabled for a month, or a burst of releases occurred), the tag is never found. In that case:
- All 10 releases are returned as "new" — but there may be more between release Unify Chat and Dashboard into single view #11 and the watermark.
- The agent then processes those 10 and updates
last_reviewed_releaseto the newest tag. - Releases Unify Chat and Dashboard into single view #11 through the old watermark are permanently skipped — their notes are never reviewed.
There's no signal to the caller that the watermark wasn't found, so this is a silent failure.
Fix suggestion: Track whether the watermark was hit and surface it:
$watermarkFound = $false
foreach ($r in $allReleases) {
if ($r.tag_name -eq $SinceTag) { $watermarkFound = $true; break }
# ... collect release ...
}
return @{
status = 'ok'
repo = $Repo
releases = $newReleases
watermark_found = $watermarkFound
}Then the caller (or the agent) can warn/paginate when watermark_found is $false.
| 2. **Classify P0-P3:** P0=factually wrong, P1=security, P2=new features, P3=nice-to-have | ||
| 3. **Only auto-fix P0 and P1.** Note P2 in PR description. Skip P3. | ||
| 4. **Update sync manifest** — `resolution_expected`, `last_reviewed`, new issues | ||
| 3. **Auto-fix P0, P1, AND P2.** Only skip P3 (cosmetic/nice-to-have). |
There was a problem hiding this comment.
🟡 MODERATE — P2 auto-fix now includes security-sensitive content without explicit human gate (Flagged by: 2/3 reviewers)
Changing Rule 3 from "Note P2 in PR description" to "Auto-fix P0, P1, AND P2" means the agent can autonomously modify the security patterns table, anti-patterns table, and trigger guide based solely on its interpretation of release notes. Rule 4 explicitly instructs it to add to "security patterns as appropriate."
The draft PR does provide a human review gate before merge, which mitigates the risk. However, the prior P2-as-note-only policy was a deliberate safeguard — if the agent misinterprets a release note, incorrect security guidance ends up in draft content that reviewers may rubber-stamp.
Suggestion: Consider keeping P2-as-note for changes that touch divergence sections or security guidance, and only auto-fix P2 changes to non-security documentation sections.
| $json = gh api "repos/$Repo/releases?per_page=10" --jq '[.[] | {tag_name: .tag_name, name: .name, published_at: .published_at, body: .body}]' 2>&1 | ||
| if ($LASTEXITCODE -ne 0) { | ||
| return @{ | ||
| status = 'error' | ||
| repo = $Repo | ||
| error = "Failed to fetch releases: $json" | ||
| } | ||
| } | ||
| if (-not $json -or $json -eq 'null' -or $json -eq '[]') { | ||
| return @{ status = 'ok'; repo = $Repo; releases = @() } | ||
| } | ||
| $allReleases = $json | ConvertFrom-Json | ||
| # Collect releases newer than $SinceTag | ||
| $newReleases = @() | ||
| foreach ($r in $allReleases) { |
There was a problem hiding this comment.
🟡 MODERATE — Silent history gap when >10 releases exist since watermark (Flagged by: 3/3 reviewers)
per_page=10 fetches at most 10 releases. If the watermark tag ($SinceTag) falls outside this window, the foreach loop never hits the break and silently returns all 10 as "new." The agent then updates last_reviewed_release to the latest tag — permanently skipping all intermediate releases beyond position 10.
Failing scenario: Watermark at v0.50, repo now at v0.80 with 20 releases since. Only the 10 newest are returned; v0.51–v0.70 are never reviewed, yet the watermark jumps to v0.80.
Suggested fix: Return a watermark_found boolean in the result so the caller can warn or paginate:
$foundWatermark = $false
foreach ($r in $allReleases) {
if ($r.tag_name -eq $SinceTag) { $foundWatermark = $true; break }
# ... existing logic
}
# Include in return:
# watermark_found = $foundWatermark| $newReleases = @() | ||
| foreach ($r in $allReleases) { | ||
| if ($r.tag_name -eq $SinceTag) { break } | ||
| $body = if ($r.body.Length -gt 5000) { $r.body.Substring(0, 5000) + '...' } else { $r.body } |
There was a problem hiding this comment.
🟡 MODERATE — Null body on release notes causes silent failure (Flagged by: 3/3 reviewers)
GitHub releases with no body have body: null. In PowerShell 7, $null.Length returns $null (not an exception), so the else branch runs and $body = $null. The agent's Rule 4 instructs it to "Read ALL release notes" — if it calls string methods on this null value, processing fails.
The same pattern exists in Get-GitHubLatestRelease (line ~129) but is lower risk there since the fallback path doesn't process notes.
Suggested fix:
$body = if ($r.body -and $r.body.Length -gt 5000) { $r.body.Substring(0, 5000) + '...' } else { if ($r.body) { $r.body } else { '' } }| $hasNewReleases = ($results | ForEach-Object { $_.sources } | Where-Object { | ||
| $_.type -eq 'releases' -and $_.result.status -eq 'ok' -and $_.result.releases.Count -gt 0 -and $_.last_reviewed_release |
There was a problem hiding this comment.
🟢 MINOR — Fallback releases (no watermark) never trigger changes_detected (Flagged by: 3/3 reviewers after follow-up)
The $hasNewReleases filter requires $_.last_reviewed_release to be truthy. When a releases: source is added without a last_reviewed_release field, the fallback path fetches the latest release and logs "add to manifest," but $hasNewReleases evaluates to $false — so changes_detected is never set from this source.
Failing scenario: A new releases: entry is added to a sync manifest without a watermark. The script logs an advisory but never triggers drift detection, so no update PR is created until someone manually adds the field.
Suggested fix: Include fallback sources in the check, or treat "missing watermark" as actionable:
$hasMissingWatermark = ($results | ForEach-Object { $_.sources } | Where-Object {
$_.type -eq 'releases' -and -not $_.last_reviewed_release -and $_.result.releases.Count -gt 0
} | Measure-Object).Count -gt 0
Three bugs fixed in drift detection:
changes_detectednever included new releasesAlso adds
last_reviewed_release: v0.69.3watermark to sync manifest.Tested locally: correctly detects v0.70.0 + v0.71.0 as new.