Repository navigation
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| runs-on: ubuntu-latest | ||
| timeout-minutes: 10 | ||
| steps: | ||
| - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4 |
There was a problem hiding this comment.
🔴 Every CI run may fail before any step runs, or run unreviewed code, if the pinned actions/checkout commit is not an upstream v4 release. .github/workflows/ci.yml:16 pins 11d5960a326750d5838078e36cf38b85af677262 with the comment # v4. That hash is not the v4.2.2 tag commit (11bd71901bbe5b1630ceea73d27597364c9af683), and it could not be confirmed against any v4 tag from this checkout (no network). Fix: pin the full commit SHA that an actions/checkout v4 release tag points to, and name the exact version in the comment (for example # v4.2.2) so reviewers and Dependabot can check it.
Why this was flagged
Condition: the SHA 11d5960a326750d5838078e36cf38b85af677262 at .github/workflows/ci.yml:16 is not a commit reachable in actions/checkout; this could not be checked from the checkout because remote access is refused. The workflow triggers on every pull_request and on push to main (.github/workflows/ci.yml:3-6). If the SHA does not exist, GitHub fails the job during set-up with "Unable to resolve action", so none of the new checks run and every PR shows a red CI. If the SHA exists only in a fork of the action's repository network, the job runs code that is not an upstream release while the comment says # v4. The base branch has no ci.yml; its other workflows use actions/checkout@ v4 (.github/workflows/claude.yml:29, .github/workflows/release.yml:16), which always resolves. No safeguard applies: the # v4 comment is not validated, and Dependabot (.github/dependabot.yml:3) only proposes later bumps. The neighbouring pin actions/setup-python@ a26af69be951a213d495a4c3e4e4022e16d87065 at line 17 does match the known v5.6.0 commit.
Verification: .github/workflows/ci.yml:16 is - uses: actions/checkout@ 11d5960a326750d5838078e36cf38b85af677262 # v4, the first step of the only job (checks, lines 12-15). The workflow runs on every pull_request and on push to main (lines 3-6). An unresolvable SHA fails the job there, so none of the steps on lines 17-36 would ever execute. No guard exists: there is no fallback ref.
Finding ID: f-556774cf
| import yaml | ||
|
|
||
| SKIP_DIRS = {".git", "node_modules", "__pycache__", ".venv"} | ||
| FENCE = re.compile(r"^ {0,3}(`{3,}|~{3,})(.*)$") |
There was a problem hiding this comment.
🟡 (optional) Maintainers get no CI failure for an unclosed code block nested in a list item, and link-like text inside such a block can fail CI as a broken link. FENCE at .github/scripts/check_repo.py:12 only accepts 0-3 leading spaces, so a fence indented 4 or more spaces is treated as prose by split_fences. Fix: recognise fences at list-item indentation for both the opening and the closing line, for example ^\s* in place of ^ {0,3}, and add a test with an indented fence. The repo already has one such block at clinical-trial-forecast/SKILL.md:59-77.
Why this was flagged
Trigger: a Markdown file with a fenced block inside a list item, indented 4 or more spaces, reached through check_markdown (.github/scripts/check_repo.py:39) from the CI step python3 .github/scripts/check_repo.py. FENCE = re.compile(r"^ {0,3}({3,}|~{3,})(.*)$")at line 12 does not match those lines, sosplit_fences(lines 24-36) never setsmarkerand appends the fence lines and everything between them tooutside. Two results follow. An opening fence with no closing fence in a list item is never reported as "code block is never closed", which is the defect class this PR fixes by hand in 04-protocol-operations.md. And text such as xinside the block is passed toLINK.findall` at line 47 and reported as "broken link to gone.md", failing CI on valid content. clinical-trial-forecast/SKILL.md:59 and :77 are fences indented 5 spaces under list item 3, so lines 60-76 are already scanned as prose; they hold no links today, so nothing fails yet. The base branch has no check at all. No test in test_check_repo.py covers an indented fence.
Verification: Triggers when a Markdown file has a fenced block indented 4 or more spaces inside a list item and that block is either left unclosed or contains link-like text with a path target. In .github/scripts/check_repo.py, line 12 FENCE only matches fences with 0-3 leading spaces. The base has no checker at all. test_check_repo.py has no indented-fence case.
Finding ID: f-67d567f9
| target = target.split("#")[0] | ||
| is_path = "/" in target or "." in target # a bare word such as (url) is a format example | ||
| if is_path and not re.match(r"[a-z][a-z0-9+.-]*:", target) and not (path.parent / target).exists(): |
There was a problem hiding this comment.
🟡 (optional) Contributors get a false "broken link to …" CI failure for Markdown links that GitHub renders correctly. At .github/scripts/check_repo.py:50 the raw link text is joined to the file's folder and tested with exists(). A root-relative link such as /README.md is looked up at the filesystem root, and my%20file.md or file.md?plain=1 are looked up literally. Fix: normalise the target before the existence test so every valid GitHub link form resolves to the file it names: drop ?query, percent-decode, resolve a leading / against root, and match the scheme case-insensitively. No file in the repository uses these forms today.
Why this was flagged
Trigger: a pull request adds a Markdown link in one of these forms outside a code block: [x](/README.md), [x](my%20file.md), [x](file.md?plain=1) or [x](HTTPS://example.com). It arrives through check_markdown (.github/scripts/check_repo.py:39), called from main at line 107 and run by the CI step at .github/workflows/ci.yml:25. Line 48 strips only the #fragment. Line 50 evaluates (path.parent / target).exists(): joining an absolute target discards path.parent, so /README.md is tested at the filesystem root; %20 and ?plain=1 stay in the file name; the scheme test re.match(r"[a-z][a-z0-9+.-]*:", target) is case-sensitive, so an upper-case scheme is treated as a path. Line 51 appends broken link to ..., main returns 1 and the job fails although the link is valid on GitHub. The base branch has no link check. The existing tests (.github/scripts/test_check_repo.py:54-60) cover only plain relative paths, lower-case web links, anchors and bare words, so none of these forms is exercised.
Verification: Triggers when a Markdown file outside a code block contains a root-relative, percent-encoded, query-suffixed or upper-case-scheme link; no file in the tree at HEAD does today, so CI is not currently broken. In .github/scripts/check_repo.py, line 48, target = target.split("#")[0], is the only normalisation. The tests in .github/scripts/test_check_repo.py (lines 50-60) pin none of these forms.
Finding ID: f-f6b61711
| for path in repo_files(root, ".yaml", ".yml"): | ||
| try: | ||
| list(yaml.safe_load_all(path.read_text(encoding="utf-8"))) | ||
| except yaml.YAMLError as error: |
There was a problem hiding this comment.
🟡 (optional) A contributor who adds a YAML file with an impossible date, or a .md/.yaml file that is not UTF-8, gets a bare traceback and loses every other reported problem. check_repo.py:65 catches only yaml.YAMLError. PyYAML raises a plain ValueError for a scalar such as 2024-02-30, and read_text at check_repo.py:63 and :43 raises UnicodeDecodeError. The JSON loop at :60 already catches ValueError, so the two loops disagree. Fix: every per-file read and parse must turn its failure into a problem line; catch (yaml.YAMLError, ValueError) at :65 and guard the read at :43.
Why this was flagged
The trigger is a data file such as released: 2024-02-30 in any .yaml/.yml, or a Latin-1 encoded .md or .yaml, added by a pull request and reached through main at .github/scripts/check_repo.py:107. yaml.safe_load_all at check_repo.py:64 builds a datetime.date for the timestamp tag and that constructor raises ValueError, which is not a yaml.YAMLError. read_text(encoding="utf-8") at check_repo.py:43 and :63 raises UnicodeDecodeError, also outside the handler. The exception escapes check_data_files or check_markdown, so the loop at check_repo.py:109-111 never prints the problems already collected by check_marketplace, and the traceback does not name the offending file. The JSON branch at check_repo.py:60 handles both cases with except ValueError. This is a different site and input from the filed check_skill item at :82. No such file exists at HEAD. Remedy: widen the handler and wrap the reads.
Verification: Triggers when a PR adds a non-UTF-8 .md/.yaml/.yml file, or a YAML file with an impossible date scalar. In .github/scripts/check_repo.py, line 63-65 is guarded only by except yaml.YAMLError, so UnicodeDecodeError escapes. An escaping exception discards every problem already collected and prints only a traceback. The test suite does not exercise these cases.
Finding ID: f-09e6e459
| grep -l -- '--self-test' */scripts/*.py | while read -r script; do | ||
| python3 "$script" --self-test | ||
| done |
There was a problem hiding this comment.
🟡 (optional) Maintainers get a green "Run skill self-tests" step even when no self-test ran. At .github/workflows/ci.yml:34 grep -l feeds a while read loop, and the step sets no shell:, so it runs as bash -e without pipefail. If grep matches nothing or errors, the loop runs zero times and the step exits 0. Fix: make the step fail when grep fails or the discovered list is empty, for example shell: bash on the step (which adds -o pipefail) or collecting the matches first and testing they are non-empty. Today only binder-gauntlet/scripts/sequence_profile.py matches, so one rename or move silently drops all coverage.
Why this was flagged
Trigger: a later change moves or renames binder-gauntlet/scripts/sequence_profile.py out of the */scripts/*.py glob, nests it one level deeper, or renames its --self-test flag (binder-gauntlet/scripts/sequence_profile.py:249); it is the only file that matches today. The step at .github/workflows/ci.yml:33-36 has no shell: key and the workflow has no defaults.run.shell, so GitHub runs it as bash -e {0}, which does not set pipefail. grep -l then exits 1 (no match) or 2 (unmatched glob passed literally), but the pipeline status is that of the while loop, which ran zero iterations and returns 0. The step is reported as passed with no self-test executed. The base branch has no CI, so the new harm is a check that reports success without having run. bash -e does not help: errexit ignores a failure in a non-last pipeline element. A failing self-test inside the loop does still fail the step; only the zero-match case is open.
Verification: Triggers only when a later change leaves no */scripts/*.py file containing --self-test; nothing fails at HEAD. In .github/workflows/ci.yml lines 32-36 the step has no shell: key, so it runs as bash -e {0} with no pipefail. If grep exits 1 or 2, the loop reads nothing and returns 0, so the step is green with zero self-tests run. The base had no CI at all.
Finding ID: f-7e6f2144
There was a problem hiding this comment.
Code review found no new issues
No new issues were found in this update; 5 findings from earlier reviews are still open above.
1 further finding on code this push did not change was outside the scope of this re-review and not posted.
Still open from earlier reviews (5):
- 🔴
.github/workflows/ci.yml:16—Every CI run may fail before any step runs, or run unreviewed code, if the pinned actions/checkout commit is not an ups… - Also unresolved: 4 minor or pre-existing.
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
Adds a CI workflow that validates the marketplace, skills, Markdown links and code blocks, JSON and YAML files, Python and shell lint, and skill self-tests. Also adds a
root LICENSE, CODEOWNERS and Dependabot for GitHub Actions, and removes a stray code fence from the clinical trial protocol skill.