Skip to content

fix(pr-management-triage): centralize terminal PR links - #1049

Merged
potiuk merged 1 commit into
apache:mainfrom
AyushS1304:issue-73-osc8-pr-links
Aug 1, 2026
Merged

fix(pr-management-triage): centralize terminal PR links#1049
potiuk merged 1 commit into
apache:mainfrom
AyushS1304:issue-73-osc8-pr-links

Conversation

@AyushS1304

Copy link
Copy Markdown
Contributor

Summary

  • Add one formatter for short and full GitHub pull-request references, resolving each to a canonical /pull/<number> URL.
  • Emit OSC 8 terminal hyperlinks when supported, with readable URL fallbacks for NO_COLOR, TERM=dumb, and non-terminal output.
  • Apply the renderer consistently across triage progress, proposals, grouped results, drill-down views, errors, and summaries, with regression fixtures for each fallback path.

Type of change

  • Skill change (.claude/skills/<name>/) — eval fixtures updated below
  • Tool / bridge contract (tools/<system>/*.md)
  • Python package (tools/*/ with pyproject.toml)
  • Groovy reference impl
  • Cross-cutting (RFC, AGENTS.md, sandbox, privacy-LLM)
  • Documentation (docs/, README.md, CONTRIBUTING.md)
  • Project template (projects/_template/)
  • CI / dev loop (prek, workflows, validators)
  • Other: triage CLI helper and unit tests

Test plan

  • prek run --all-files passes
  • For Python packages touched: uv run pytest / ruff check / mypy passes
  • For Groovy bridges touched: command-line invocation tested end-to-end
  • For skill changes: eval suite passes for the affected skill
  • For skill behaviour changes: a new or updated eval fixture is included in this PR
  • Other: 9 helper unit tests and all 150 skill-eval framework tests pass

RFC-AI-0004 compliance

  • HITL — any new mutation is gated on explicit user confirmation
  • Sandbox — no new unrestricted host access; network reach declared in the adapter
  • Vendor neutrality — placeholders used in skill and tool prose
  • Conversational + correctable — agentic-override path documented if behaviour is adopter-tunable
  • Write-access discipline — no autonomous outbound messages; drafts only, sent on confirmation
  • Privacy LLM — private content does not reach a non-approved LLM; redactor invoked where needed

Ai Assistance

took help from Codex for testing.

Linked issues

Closes #73

Notes for reviewers

The helper deliberately keeps the visible reference compact in capable terminals while ensuring the canonical URL remains visible everywhere else.

Use one formatter for canonical pull-request targets and terminal fallback behavior across every triage output path.

Generated-by: Codex (GPT-5)

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM on the change itself — centralising this is right, and the fallback matrix is the part most implementations get wrong. One structural note about where the tests live, which is not really about this PR but lands here because this PR is the first to hit it.

The change is sound

Golden rule 10 in SKILL.md requires every PR number to render as a full clickable URL, and until now each surface did that by hand. One formatter that resolves both short (#1049) and full-URL forms to a canonical /pull/<number> is the correct shape, and applying it across triage progress, proposals, grouped results, and dry-run output is what makes the rule actually hold rather than hold in the places someone remembered.

The fallback handling is the good part. OSC 8 hyperlinks degrade badly when emitted blindly — NO_COLOR, TERM=dumb, and non-terminal output each need the readable URL instead of an escape sequence that renders as line noise in a log or a piped file. All three have their own fixture (case-3-no-color, case-4-dumb-terminal, plus the non-tty path), which is more care than this usually gets.

I ran the tests: 9 passed. mypy is clean on pr_link.py.

The tests will not run in CI

skills/pr-management-triage/tests/test_pr_link.py is the first test file anywhere under skills/ — there are currently zero others. The workspace checks that run ruff, mypy, and pytest are scoped to uv workspace members, and their hook is filed under ^(tools/[^/]+(/[^/]+)?/(src|tests|pyproject\.toml)|pyproject\.toml|…). skills/** matches none of it.

So on merge:

  • those 9 tests never execute in CI — revert pr_link.py tomorrow and the checks stay green;
  • the 133-line script is neither linted nor type-checked there.

Not hypothetical: running ruff over the new files by hand already finds RUF100 Unused 'noqa' directive (unused: E402) at tests/test_pr_link.py:26. Trivial in itself — the point is that it is exactly the class of thing the repo lints for everywhere else and would have flagged automatically had the path been covered.

This is the third instance of the same shape this week: #1011 shipped a regression test that nothing collected (fixed by making ai-tutors/ a workspace member), and #982 / #1043 dealt with eval cases silently falling back to MANUAL. Different mechanisms, same failure: a check that looks present and never runs.

I am not asking you to fix it here. The ai-tutors remedy — make the directory a uv workspace member — is the wrong shape for skills/, since skills are agent-facing markdown plus helper scripts that get symlinked into adopter repos, not Python packages. The right answer is probably a single pytest invocation over skills/**/tests/ wired into the existing job, and that is a maintainer decision about repo structure rather than something to bolt onto a feature PR. Worth its own issue, and I will raise one.

The noqa is worth deleting while you are here, though, since it is already stale.


This review was drafted by an AI-assisted tool and
confirmed by an Apache Magpie maintainer. The maintainer
approving this PR has read the findings and signed off. If
something feels off, please reply on the PR and a maintainer
will follow up.

More on how Apache Magpie handles maintainer review:
CONTRIBUTING.md § Opening a pull request.

@potiuk
potiuk merged commit e33dacf into apache:main Aug 1, 2026
10 checks passed
@AyushS1304

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review and for merging this. The skills/** CI gap is a great catch. I saw #1053 and #1054 cover the broader fix, including the stale noqa. Appreciate you tracing it through and wiring the checks up properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Triage CLI: render PR references as clickable terminal hyperlinks (OSC 8)

2 participants