Encode history-link query quotes to prevent footer markdown link breakage - #53978
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Encodes history-link search queries to prevent Markdown parsing failures.
Changes:
- Explicitly percent-encodes search queries with
+space normalization. - Adds regression coverage for encoded marker quotes.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/generate_history_link.cjs |
Updates history URL serialization. |
actions/setup/js/generate_history_link.test.cjs |
Tests quote encoding. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
| } | ||
| })(); | ||
| url.searchParams.set("q", queryParts.join(" ")); | ||
| const encodedQuery = encodeURIComponent(queryParts.join(" ")).replaceAll("%20", "+"); |
There was a problem hiding this comment.
Addressed in 0c1e1ca: query encoding now additionally RFC3986-encodes characters left by encodeURIComponent (including parentheses), preserving markdown-safe link targets.
|
@copilot This PR looks close, but it still needs a maintainer-facing finish pass.
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #53978 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100).
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix is correct and well-scoped. Switching from url.searchParams.set() to manual url.search assignment with encodeURIComponent ensures double-quote characters are encoded as %22, preventing markdown footer link breakage.
searchTypevalues are hardcoded strings — no injection risk.encodedQueryis already percent-encoded; no double-encoding occurs.- New test clearly documents and validates the expected encoding behaviour.
LGTM
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 21.2 AIC · ⌖ 8.8 AIC · ⊞ 5.7K
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.
|
There was a problem hiding this comment.
Request changes
The quote-encoding fix is directionally right, but the implementation regresses the existing query encoding rules and can break history searches for workflow IDs containing parentheses.
Blocking theme
- Replacing
URLSearchParamswithencodeURIComponent(...).replaceAll("%20", "+")is not behavior-preserving. (and)are no longer escaped, while workflow IDs come from filenames and can legally include those characters.- That means some generated history links will stop matching the exact quoted marker they were supposed to search for.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 5.47 AIC · ⌖ 6.92 AIC · ⊞ 7K
Comment /review to run again
| } | ||
| })(); | ||
| url.searchParams.set("q", queryParts.join(" ")); | ||
| const encodedQuery = encodeURIComponent(queryParts.join(" ")).replaceAll("%20", "+"); |
There was a problem hiding this comment.
This manual encoding changes the query semantics: encodeURIComponent leaves ( and ) unescaped, so workflow IDs derived from filenames like build (nightly).md will produce a different q= than URLSearchParams did and can stop matching the exact quoted marker in GitHub search.
💡 Keep the markdown-safe quote fix without regressing other characters
The previous implementation was correctly delegating query-string encoding to URLSearchParams; the only broken part was the raw " delimiter in the final markdown link target. A safer fix is to keep searchParams for encoding and then post-process only the quote characters in the serialized URL, or explicitly encode with the same rules as form-urlencoding instead of encodeURIComponent.
url.searchParams.set("q", queryParts.join(" "));
url.searchParams.set("type", searchType);
// If markdown safety still requires it, normalize only raw quotes in the final URL
return url.toString().replaceAll('"', '%22');That preserves the established encoding behavior for parentheses and any other characters GitHub search expects while still removing the raw quotes that break markdown rendering.
There was a problem hiding this comment.
Addressed by 0c1e1ca (RFC3986 encoding step) and validated by the parentheses regression test in actions/setup/js/generate_history_link.test.cjs.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — the fix is correct and well-targeted; one minor test coverage suggestion.
📋 Key Themes & Highlights
Key Themes
- Root cause properly addressed: switching from
URLSearchParams.set()to manualencodeURIComponent()ensures quote characters are encoded as%22rather than left raw in the URL, which was causing GitHub Markdown to misparse the link destination boundary. - Regression test present: the new test asserts both the positive case (
%22...%22wrapper is present) and a negative case (raw+"gh-aw-workflow-call-idis absent). One suggestion: the negative assertion is narrowly scoped (see inline comment). - Existing concern (already raised):
encodeURIComponentleaves(and)unencoded, unlikeURLSearchParams. If workflow IDs can contain parentheses, this could produce a different kind of markdown breakage.
Positive Highlights
- ✅ Minimal, surgical change — only the URL construction lines are touched
- ✅ Regression test added alongside the fix
- ✅ PR description clearly explains the root cause and the encoding contract
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 26.3 AIC · ⌖ 9.95 AIC · ⊞ 7.8K
Comment /matt to run again
| itemType: "issue", | ||
| workflowCallId: "elastic/docs-eng-team/gh-aw-issue-auto-triage", | ||
| serverUrl: "https://github.com", | ||
| }); |
There was a problem hiding this comment.
[/tdd] The negative assertion only guards against one specific raw-quote pattern; if the query structure changes, a raw " could sneak back in undetected.
💡 Stronger guard
Replace the narrow not.toContain with a blanket assertion that no unencoded quote survives:
expect(url).not.toContain('"');This directly enforces the invariant the fix is meant to provide, regardless of where in the query a raw quote might appear.
@copilot please address this.
There was a problem hiding this comment.
Implemented in c55026b: strengthened the test to assert expect(url).not.toContain('"'), so any raw quote in the generated URL now fails the test.
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Implemented in Maintainer handoff:
Remaining risk:
|
PR Triage
Batch: B1-small-fixes-refactors Tiny fix (add:32/del:2) encoding query quotes in footer links. Good candidate for grouped review with other small B1 fixes. Automated triage by PR Triage Agent.
|
|
@copilot merge main and lint js |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…fix-footer-related-runs-link Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in
|
|
@copilot Sous-chef triage: please investigate this PR and move it toward merge readiness. Refresh the branch if needed, review any unresolved feedback, and run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done in
Validation run:
CI snapshot:
Remaining risk / handoff:
|
|
@copilot quick triage for this PR: Unresolved review threads:
Please refresh the branch if needed, address the items above, and then run the
|
Triage pass complete.
Validation run in this pass:
CI/log triage via MCP:
No additional code changes were required in this pass. |
PR TriageCategory: bug | Risk: low | Priority: low (score 37/100)
|
|
@copilot Please do a maintainer-facing finish pass on this PR.
|
Finish pass completed on current HEAD
Remaining merge risks:
|
PR TriageCategory: Small fix, CI passing, but flagged needs-rework in a prior triage pass — verify before promoting. Automated triage — see the triage report issue for full context.
|
|
🎉 This pull request is included in a new release. Release: |
The footer’s related-runs link could render as raw markdown because the search query contained an unencoded opening
"in the URL. GitHub markdown parsing then misread the link destination/title boundary and dropped link rendering.History URL construction
actions/setup/js/generate_history_link.cjsto buildqwith explicit percent-encoding and+space normalization.%22...%22(no raw quote characters in the markdown link target).Regression coverage
actions/setup/js/generate_history_link.test.cjsasserting:%22gh-aw-workflow-call-id...%22is present+"gh-aw-workflow-call-idis absentRun: https://github.com/github/gh-aw/actions/runs/32326521694> Generated by 👨🍳 PR Sous Chef · gpt54 · 11.9 AIC · ⌖ 8.15 AIC · ⊞ 9.3K · ◷