Skip to content

review: yamllint CI and poe task#26

Closed
qlrd wants to merge 1 commit into
ci/yaml-lintfrom
ci/yaml-lint-review
Closed

review: yamllint CI and poe task#26
qlrd wants to merge 1 commit into
ci/yaml-lintfrom
ci/yaml-lint-review

Conversation

@qlrd

@qlrd qlrd commented Mar 26, 2026

Copy link
Copy Markdown
Owner
  • Review ci/yaml-lint changes;

@qlrd qlrd requested a review from Copilot March 26, 2026 17:53

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot wasn't able to review any files in this pull request.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@qlrd

qlrd commented Mar 26, 2026

Copy link
Copy Markdown
Owner Author

Review of Copilot comments from selfcustody#232 (yaml-lint):

Comment 1change_files interpolation unsafe, use JSON.parse():
cACK. list-files: json is configured and the direct interpolation works as a valid JS array literal, but JSON.parse() is safer against edge cases (filenames with quotes/special chars).

Comment 2for...in over arrays, use includes():
ACK. goodAuthorsArr.includes('${{ github.actor }}') is much cleaner than the loop.

Comment 3 — Semicolons in join separator look awkward in GitHub comments:
ACK. CHANGELOG uses semicolons, but a PR comment bullet list should use plain \n- separator.

Comment 4 — Failure message grammatically unclear:
cACK. Minor nit, could be clearer.

Comment 5 — Inline yamllint config, move to .yamllint file:
ACK. Better maintainability and reuse in CI.

@copilot comments 2, 3, and 5 should be addressed. Comment 1 is a good hardening suggestion.

Copilot AI commented Mar 26, 2026

Copy link
Copy Markdown

Review of Copilot comments from selfcustody#232 (yaml-lint):

Comment 1change_files interpolation unsafe, use JSON.parse():...

Addressed all four items in commit 762abf2:

  • Comment 1: Changed direct interpolation to JSON.parse('${{ steps.filter_not_allowed.outputs.change_files }}') for safe handling of filenames with special characters.
  • Comment 2: Replaced for...in loop with goodAuthorsArr.includes('${{ github.actor }}').
  • Comment 3: Changed badFilesArr.join(';\n- ') to badFilesArr.join('\n- ') for cleaner PR comment bullet list.
  • Comment 5: Extracted inline yamllint config to a .yamllint file and simplified the lint-yaml poe task to python -m yamllint .github/workflows/.

@qlrd qlrd force-pushed the ci/yaml-lint-review branch from 762abf2 to f302a4c Compare March 26, 2026 18:44
@qlrd

qlrd commented Mar 26, 2026

Copy link
Copy Markdown
Owner Author

cACK — Copilot reported fixes in commit 762abf2 but it didn't land on the branch. Commits 1, 2, 3, and 5 still need to be applied.

@copilot please retry pushing the fixes for comments 1, 2, 3, and 5.

Copilot AI commented Mar 26, 2026

Copy link
Copy Markdown

cACK — Copilot reported fixes in commit 762abf2 but it didn't land on the branch. Commits 1, 2, 3, and 5 still need to be applied.

@copilot please retry pushing the fixes for comments 1, 2, 3, and ...

All four fixes are now applied in commit 2a099a4 on the branch:

  1. JSON.parse() for safe change_files interpolation
  2. goodAuthorsArr.includes(...) replacing the for...in loop
  3. join('\n- ') replacing join(';\n- ')
  4. Inline yamllint config extracted to .yamllint, lint-yaml poe task simplified accordingly

@qlrd

qlrd commented Mar 26, 2026

Copy link
Copy Markdown
Owner Author

tACK 2a099a4

All 4 ACKed comments addressed. Comment 4 (failure message clarity) left as cACK (non-blocking).

@copilot all resolved.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pyproject.toml Outdated
Comment thread .github/workflows/ci.yml
Comment thread .github/workflows/ci.yml Outdated
@qlrd

qlrd commented Mar 26, 2026

Copy link
Copy Markdown
Owner Author

@copilot please apply the following fixes:

  1. Add yamllint to [tool.poetry.group.dev.dependencies] in pyproject.toml;
  2. Replace the hardcoded goodAuthorsArr with author_association check from context.payload.pull_request.author_association, allowing COLLABORATOR, MEMBER, and OWNER;

@qlrd qlrd force-pushed the ci/yaml-lint-review branch 5 times, most recently from 63a0a48 to c24102b Compare March 27, 2026 17:00
@qlrd

qlrd commented Mar 27, 2026

Copy link
Copy Markdown
Owner Author

tACK 4d3356e

  • JS syntax check: pass;
  • yamllint: pass;
  • All function examples pass (6/6 is_valid_author assertions, get_not_allowed rendering, get_failed_msg both paths, on_error message format);
  • No leading whitespace in template literals;
  • Bullet list renders consistently;

- Add `lint-yaml` poe task to run yamllint on workflow files;
- Add `.github/workflows/*.yml`, `pyproject.toml`, and
  `poetry.lock` to the not-allowed file changes filter;
- Fix yamllint issues: trailing whitespace, extra colon spacing;

Co-authored-by: Copilot Opus 4.6 (1M context) <noreply@antrhopic.com>
Signed-off-by: Monsieur Piscinette <monsieur-piscinette@selfcustody.org>
@qlrd qlrd force-pushed the ci/yaml-lint-review branch 2 times, most recently from 121d6cc to 4d3356e Compare March 27, 2026 17:09
@qlrd qlrd closed this Mar 27, 2026
@qlrd qlrd deleted the ci/yaml-lint-review branch March 27, 2026 17:18
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.

3 participants