ci: allowlist ECMA-48 in the public-repo-hygiene caller - #261
Open
mattmillerai wants to merge 3 commits into
Open
ci: allowlist ECMA-48 in the public-repo-hygiene caller#261mattmillerai wants to merge 3 commits into
mattmillerai wants to merge 3 commits into
Conversation
The caller was pinned at 4df9bf6 since it landed, one day before the checker's known-public allowlist gained comfy-skills and workflow_templates, so every PR here failed on five references the current checker accepts. The pin alone leaves two findings: ECMA-48, the ANSI escape standard cited in clitext.py and test_compat.py, has the letters-digits shape the checker treats as a ticket id and no built-in prefix covers it. Both changes are needed for the check to pass; neither passes on its own.
|
Warning Review limit reachedNext included review available in 38 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 123 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Comment |
mattmillerai
changed the title
The pin bump landed separately in #260; this branch now carries only the ticket_allowlist entry.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 3 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 1 |
| 🟢 Low | 2 |
Panel: 8/8 reviewers contributed findings.
Review of the ECMA-48 allowlist entry flagged two misleading comments. The header said "the allowlist is deliberately NOT an input" without naming which one, which the new ticket_allowlist line made read as false; it now says the known-public repo/team allowlist is the one that is not an input, and describes the ticket allowlist for what it is: an additive, category-1-only knob whose every entry sits in this file's diff, so the diff review is the control the reusable's header asks for. The entry's own comment now records that the value is matched as an exact token (a bare ECMA prefix suppresses nothing, checked against the checker at the pinned SHA) instead of the earlier "can only widen it" wording that read as reassurance. No behavior change: ticket_allowlist stays ECMA-48, the checker at the pinned SHA still reports 0 findings on this tree. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ELI5
This repo runs a shared "did anything internal leak into this public repo" scanner. It reads
ECMA-48(the terminal escape-code standard our ANSI parser cites in two comments) as an internal ticket number, because it has the same LETTERS-DIGITS shape. This tells the scanner that one token is a standards number, not a ticket. It is the last thing keeping the check red onmain.Motivation
hygiene / public-repo-hygienehas been red onmainand on every PR since #254 — ci: adopt the shared public-repo-hygiene workflow merged on 2026-08-22. Seven findings: five were references tocomfy-skillsandworkflow_templatesthat the checker's known-public allowlist gained the next day (Comfy-Org/github-workflows#212 — fix(public-repo-hygiene): allowlist six verified-public Comfy-Org repos), cleared by the pin bump in #260 — ci: bump public-repo-hygiene to github-workflows@a02bb3e. The remaining two are theECMA-48citations insrc/comfy_mcp/clitext.pyandtests/test_compat.py, which no built-in prefix (RFC/ISO/PEP/CVE) covers. Until they are allowlisted, every PR here, such as #253 — ci: bump cursor-review to github-workflows@27e23ac, carries a failing required-looking check that has to be overridden by hand.Provenance
Comfy-Org/github-workflowsmainagainst this branch with--ticket-allow ECMA-48: 0 findings. Control run without the flag: 4 findings, allECMA-48(the two source comments plus the two new lines in this caller).yaml.safe_loadparses the caller andjobs.hygiene.with.workflows_refequals theuses:SHA. This PR's ownhygiene / public-repo-hygienerun passed before ci: bump public-repo-hygiene to github-workflows@a02bb3e #260 merged, when the branch also carried the pin bump. Not run:pytestandruff, no Python changed.origin/mainwas merged in and this PR now adds only the allowlist entry.Reviewer context
mainand on every open PR once they pick upmain.ECMA-48comments themselves. Rewording them to dodge the pattern would trade an accurate citation for a checker quirk; the reusable exposesticket_allowlistfor exactly this case.Summary
ticket_allowlist: ECMA-48to the caller'swith:block, with a comment explaining why; the input is additive on top of the built-in allowlist and can only widen itTest plan
hygiene / public-repo-hygieneis green on this PRmainis green