Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
🔎 PR Code Quality Reviewer is reviewing code quality for this pull request... |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Footer-disabled reviews still omit provenance, preventing subsequent supersession.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Adds durable workflow provenance to PR reviews so superseded blocking reviews can be identified after GitHub removes HTML markers.
Changes:
- Adds encoded Markdown provenance markers and exact matching.
- Uses durable markers when superseding reviews.
- Adds regression coverage for encoding and workflow isolation.
| File | Description |
|---|---|
actions/setup/js/generate_footer.cjs |
Generates and matches durable markers. |
actions/setup/js/generate_footer.test.cjs |
Tests marker encoding and matching. |
actions/setup/js/pr_review_buffer.cjs |
Adds markers and uses them during supersession. |
actions/setup/js/pr_review_buffer.test.cjs |
Tests review submission and isolation. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| const callerWorkflowId = process.env.GH_AW_CALLER_WORKFLOW_ID || ""; | ||
| if (callerWorkflowId) { | ||
| body += "\n" + generateWorkflowCallIdMarker(callerWorkflowId); | ||
| body += "\n" + generateWorkflowCallIdMarker(callerWorkflowId) + "\n" + generateWorkflowCallIdReviewMarker(callerWorkflowId); |
There was a problem hiding this comment.
Fixed in 0e9a9ec: caller provenance markers are now emitted whenever GH_AW_CALLER_WORKFLOW_ID is set, independent of shouldAddFooter/footerContext. Added a test for footer: none with an empty REQUEST_CHANGES body.
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict
I didnt find a blocking regression in the changed lines.
What I checked
- Durable marker generation and round-trip matching.
- Exact-line matching to avoid accidental supersession.
- Added tests covering encoding and cross-workflow isolation.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 40.8 AIC · ⌖ 6.99 AIC · ⊞ 20.3K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — solid regression coverage for the new durable marker, with a couple of maintainability/documentation nits.
📋 Key Themes & Highlights
Key Themes
- Marker-matching duplication:
matchesWorkflowCallIdre-implements the line-split/trim/compare pattern that likely already exists formatchesWorkflowId. A shared primitive would reduce drift as more marker formats are added. - Dual-marker write is undocumented: The review body now always carries both the legacy HTML-comment marker and the new durable Markdown-reference marker. It's unclear from the code whether this is permanent (for older-review compatibility) or a migration step to be removed later — worth a comment or follow-up note.
- No changeset: This repo uses
.changeset/*.mdfor patch notes (confirmed via existing changesets likepatch-add-workflow-call-id-marker.md), but this PR doesn't add one. Given this fixes a real bug (#63264) affectingsupersede-older-reviews, a changeset entry would help release notes.
Positive Highlights
- ✅ Good regression coverage: exact-match tests for legacy marker, durable marker, case-insensitive percent-encoding, non-standalone-line rejection, and cross-workflow isolation (
CallerAvsCallerB). - ✅ Defensive
decodeURIComponentwrapped in try/catch — malformed percent-encoding fails closed (returnsfalse) rather than throwing, verified manually. - ✅ Root cause is correctly addressed at the source (encoding scheme that survives GitHub's HTML-comment stripping in review bodies) rather than patched around symptoms.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 68.6 AIC · ⌖ 15.7 AIC · ⊞ 10.1K
Comment /matt to run again
| * @param {string} callerWorkflowId - Calling workflow identifier | ||
| * @returns {boolean} Whether the review belongs to the calling workflow | ||
| */ | ||
| function matchesWorkflowCallId(body, callerWorkflowId) { |
There was a problem hiding this comment.
[/codebase-design] matchesWorkflowCallId duplicates the same body-scanning logic that matchesWorkflowId (below, unseen in this diff) already implements for the legacy marker. Consider factoring a shared matchesExactMarkerLine(body, marker) helper so both functions share one line-matching primitive instead of re-implementing split/trim/compare independently, reducing future drift when a third marker format is added.
💡 Suggested shape
function matchesExactMarkerLine(body, marker) {
if (!body || !marker) return false;
return body.split((r/redacted)?\n/).some(line => line.trim() === marker);
}Then matchesWorkflowCallId could call this for the legacy check and layer the durable-marker regex check on top.
@copilot please address this.
There was a problem hiding this comment.
Done in 0e9a9ec: added someMarkerLine/matchesExactMarkerLine helpers. matchesWorkflowCallId now uses them for both the legacy check and the durable regex check. I left matchesWorkflowId as it is because it uses substring matching on purpose for the combined XML marker format.
| const callerWorkflowId = process.env.GH_AW_CALLER_WORKFLOW_ID || ""; | ||
| if (callerWorkflowId) { | ||
| body += "\n" + generateWorkflowCallIdMarker(callerWorkflowId); | ||
| body += "\n" + generateWorkflowCallIdMarker(callerWorkflowId) + "\n" + generateWorkflowCallIdReviewMarker(callerWorkflowId); |
There was a problem hiding this comment.
[/tdd] Both markers (legacy HTML comment + durable Markdown reference) are now always appended together, doubling provenance metadata in every review body going forward. Is the legacy <!-- gh-aw-workflow-call-id --> marker still needed once the durable one is in place, or is this only kept for a migration window? If it's a permanent dual-write, a comment explaining why (e.g. backward compatibility with older superseding logic reading old reviews) would help future readers; if temporary, consider tracking removal in a follow-up issue/changeset.
@copilot please address this.
There was a problem hiding this comment.
This is a permanent dual-write. Other safe outputs share the legacy HTML marker and it is still read from older reviews. The durable marker is needed because GitHub strips HTML comments from submitted review bodies. I added a code comment explaining this in 0e9a9ec.
There was a problem hiding this comment.
Impeccable Skills Review — Selected modes: harden, audit (bug_fix: matcher/marker regression fix)
Reviewed the diff manually (line-by-line + a standalone Node harness replicating matchesWorkflowCallId's exact-line matching, percent-decoding, and error handling) since npm ci in this sandbox failed on a self-signed-cert network restriction and vitest could not be installed/run.
Findings: none blocking.
matchesWorkflowCallIdcorrectly requires an exact trimmed-line match for both the legacy HTML marker and the new durable[gh-aw-workflow-call-id]: # "..."reference marker, so a marker embedded mid-sentence (e.g. "Quoted [...]") is correctly rejected — verified against the test'sQuoted ...case.- Percent-decoding via
decodeURIComponentis wrapped in try/catch, so a malformed encoded value degrades safely to no-match instead of throwing. - The old
workflowCallMarker/body.includes(...)substring-matching logic (which could have false-matched a caller ID that is a prefix of another) was fully removed with no orphaned references left inpr_review_buffer.cjs. - Both markers are appended together in
submitReview, preserving backward compatibility for any external tooling still scanning for the legacy HTML comment. - Test coverage added is good: durable marker generation/encoding, exact-line matching (including case-insensitive percent-encoding and cross-workflow isolation), and an integration-level supersede test using the new durable marker.
No inline comments needed — no actionable issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 61.7 AIC · ⌖ 13.2 AIC · ⊞ 8.1K
|
|
Ran
All three threads were already resolved in 0e9a9ec (verified again this pass — |
|
|
I re-ran Review threads: there are three open threads, and I replied to all of them in 0e9a9ec:
They are still marked unresolved because I have no tool to resolve threads. Checks: only What a maintainer needs to do:
|
|
@copilot There is still forward progress needed on this PR. The PR looks close, but the remaining step is maintainer-facing cleanup:
No new failed checks are listed in the compact queue, so the focus is resolving the already-addressed review feedback and finishing the review state cleanup.
|
I did a final
What a maintainer needs to do:
|
|
|
Final pass on head Review threads — all ready to resolve:
Verified after the main merge: Maintainer-only steps left:
|
|
@copilot There is still forward progress needed on this PR. Please continue with the remaining unresolved review items, newest first:
Then refresh the branch if needed and run the
|
I reran The thread still shows as unresolved only because I can't resolve review threads. A maintainer needs to:
|
|
|
I re-checked head Review thread: CI: the full CI still needs a human to start it. On this head only Steps for a maintainer:
|
GitHub strips HTML comment markers from submitted PR review bodies, preventing
supersede-older-reviewsfrom identifying prior blocking reviews created by the same workflow.Durable provenance
Safe supersession matching
pr-sous-chefbranch refresh requested from run https://github.com/github/gh-aw/actions/runs/36221620757pr-sous-chefbranch refresh requested by https://github.com/github/gh-aw/actions/runs/36255219324pr-sous-chefBranch refresh requested by PR Sous Chef run: https://github.com/github/gh-aw/actions/runs/36260722205
Branch refresh requested by pr-sous-chef run https://github.com/github/gh-aw/actions/runs/36277822788
Branch refresh requested by PR Sous Chef run https://github.com/github/gh-aw/actions/runs/36280932883
Run: https://github.com/github/gh-aw/actions/runs/36291473147
pr-sous-chefRun: https://github.com/github/gh-aw/actions/runs/36296973970
pr-sous-chefRun: https://github.com/github/gh-aw/actions/runs/36307747288
pr-sous-chefRun: https://github.com/github/gh-aw/actions/runs/36310278424