Skip to content

Lint the Python scripts and test teams-copy in CI - #180

Merged
mattmenefee merged 2 commits into
mainfrom
lint-and-test-python
Oct 24, 2026
Merged

mattmenefee merged 2 commits into
mainfrom
lint-and-test-python

Conversation

@mattmenefee

@mattmenefee mattmenefee commented Oct 24, 2026 •

Copy link
Copy Markdown
Owner

Summary

  • Turn on super-linter's Ruff linter (VALIDATE_PYTHON_RUFF), which lints unfilled-blocks.py.
  • Add a CI step that runs Ruff on teams-copy by path. Super-linter routes an extensionless file by what file reports, and the file in its image classifies teams-copy as an HTML document, so super-linter's Ruff run never sees it. The step runs the Ruff inside super-linter's own image, with the tag read from the super-linter step's uses: line, so the two runs cannot drift apart in version or default rules.
  • Add a root .ruff.toml, which both Ruff runs read. It sets the repository's 100-column limit and enables E501, which Ruff's default rules leave out, plus the W and I rules. Both scripts already pass.
  • Add test/test-teams-copy.sh, a fixture test in the same style as test-unfilled-blocks.sh. It runs teams-copy --print-html, which skips the macOS clipboard, and covers headings, every list marker, list grouping and numbering, spacer placement, inline markup and escaping, code spans, link addresses, mentions, each people-file error and drafts read from stdin or a path. HOME points at a temporary directory, so the real people file is never read.
  • Format unfilled-blocks.py with Ruff under the new config, and join each test-unfilled-blocks.sh case that fits in 100 columns onto one line.
  • Leave CI's ruff format check and mypy off. The formatter would requote every string in teams-copy, and neither script has the type annotations mypy needs.

The test lives in a top-level test/ directory rather than beside teams-copy, because homesick links each file in home/.local/bin into ~/.local/bin, which is on PATH.

Test plan

  • bash test/test-teams-copy.sh passes all 30 cases locally
  • The test catches regressions: against scratch copies of teams-copy, each of these mutants fails at least one case: removing link-address protection, loading the people file eagerly, ignoring the path argument, dropping the + or ) list markers, restricting headings to #, dropping either & unescape, removing NUL stripping, handling mentions before code spans, removing the italics whitespace guards, removing the people-file shape check and a crash (whose traceback the failure now prints)
  • uvx ruff@0.15.17 check passes both scripts with the new .ruff.toml, and reports E501 on a 106-column line under it
  • Super-linter's own Ruff run receives only unfilled-blocks.py: a debug re-run of CI on an earlier head logged "Detected file type for …/teams-copy: HTML document" and "There are 1 items to lint for PYTHON_RUFF"
  • shellcheck test/test-teams-copy.sh and yamllint -c .yaml-lint.yml .github/workflows/ci.yml are clean
  • The formatted unfilled-blocks.py parses to the same syntax tree as before, uvx ruff@0.15.17 check and format --check pass on it, and bash home/.claude/skills/reflow/scripts/test-unfilled-blocks.sh passes all 38 cases; test-unfilled-blocks.sh differs from before only in line breaks
  • CI passes on f39445f, including the teams-copy test and the Ruff step that runs super-linter's image on teams-copy

@mattmenefee mattmenefee self-assigned this Oct 24, 2026
@mattmenefee
mattmenefee force-pushed the lint-and-test-python branch 4 times, most recently from 197f2fd to d88344e Compare October 24, 2026 19:48
@mattmenefee

Copy link
Copy Markdown
Owner Author

Document Review: 3 files on lint-and-test-python (lint-and-test-python-DOC-REVIEW.md) — complete

25 findings: 20 fixed, 1 deferred, 4 observations

Redacted before posting: none

History was rewritten after this review: the branch is now d88344e, so file and line references, and the cited SHAs ef29aa6, e53efd9 and 197f2fd, may have drifted.

Click to expand full details

Document Review: 3 files on lint-and-test-python

Scope The changes on lint-and-test-python since a41d887 (origin/main), with the branch's commit message and the body of pull request #180
Branch lint-and-test-python
Reviewed 2026-10-24
Reviewed by Opus 5.5 (claude-opus-5-5) — code-best-practices-reviewer, test-suite-architect and documentation-expert
Collated by Opus 5.5 (claude-opus-5-5)

Documents

Document Commit
.github/workflows/ci.yml ef29aa6
.ruff.toml ef29aa6
test/test-teams-copy.sh ef29aa6
commit message ef29aa6
PR body #180 at ef29aa6

Review History

2026-10-24 — Initial review by Opus 5.5 (claude-opus-5-5)

The branch changes no document files, so /doc-review's scope filter would have found nothing to review. This file follows its shape, with three expert reviewers in place of one documentation-expert per document: code-best-practices-reviewer on ci.yml and .ruff.toml, test-suite-architect on test/test-teams-copy.sh and documentation-expert on the prose (comments, case descriptions, commit message and PR body). The code findings use two categories /doc-review lacks, Correctness and Test Coverage. The orchestrator collated the reports itself rather than through a collator agent, and re-ran the checks marked under Verification.

2026-10-24 — F6 ruled and settled by Opus 5.5 (claude-opus-5-5) (F6 fixed; F25 added)

F6 was ruled (b). The debug re-run of CI run 40123162318 showed super-linter classifies teams-copy as HTML and lints only unfilled-blocks.py, which F25 records; G7 opens for it ahead of G3.

2026-10-24 — Fixes applied by Opus 5.5 (claude-opus-5-5) (F1–F5, F7–F14, F17–F21 and F25 fixed)

F25 ruled (a). Every Implement finding was fixed in the amended commit e53efd9 (was ef29aa6) and the edited PR body; each test finding was checked against the mutant it names in a scratch copy, and CI run 40125228908 passed on e53efd9. F15, recommended Defer, stays open. The Documents table keeps ef29aa6, the revision the reviewers read.

2026-10-24 — Follow-up by Opus 5.5 (claude-opus-5-5) (F25's fix revised)

F25's step now runs Ruff from super-linter's image rather than a separately pinned pipx install, and the expect calls in test/test-teams-copy.sh were refilled to 100 columns at the user's request; both landed in 197f2fd (was e53efd9), and CI run 40125954265 passed on it.

2026-10-24 — F15 ruled by Opus 5.5 (claude-opus-5-5) (F15 deferred)

During /ship-it, the user ruled F15 Defer. The branch's commits were squashed into d88344e (was 197f2fd, plus a comment refill), so line references may have drifted.

Overall Assessment

The CI change itself is sound: super-linter v8.7.0 resolves the root .ruff.toml, pins Ruff 0.15.17 and passed on ef29aa6, and no README, CLAUDE.md or skill describes CI's steps, so nothing is stale. The findings fall into three clusters. First, the new teams-copy test proves less than it appears to: one case passes with the protection it names removed, one depends on case order, and the file-path entry point, several list and heading forms and some error paths have no case (G3, G4, G5). Second, the commit message and PR body overstate what ruff format would change and mislabel the mutation that was run; the commit message becomes permanent at merge (G2). Third, and most important, F6's debug re-run showed that super-linter never lints teams-copy — Ubuntu's file classifies it as HTML — so the pull request's central "no extra step" claim is false; F25 was ruled (a) and fixed with an explicit Ruff step. Every Implement finding is now fixed in 197f2fd; F15 (--help) was deferred by the user.

Formatting

F1 🟢 Low Priority - The commit body is not refilled to 72 columns ✅ Fixed

  • Document — commit message
  • Location — paragraph 1, line 2; paragraph 2, line 5; paragraph 3, line 1
  • Issue — The body wraps short of the 72 columns /commit sets: "detector" fits on paragraph 1's second line (62 columns today), "Both" on paragraph 2's fifth and "the" on paragraph 3's first. The user-wide rule is to refill so every line takes as many words as fit.
  • Suggestion — Refill the three paragraphs greedily at 72, keeping `ruff format` on one line, and check with LC_ALL=en_US.UTF-8 grep -n '.\{73,\}'.
  • Recommendation — Implement — line breaks only, and cheap before the message becomes permanent
  • Verification — git log -1 --format=%B | awk '{ print length }': line 2 is 62 columns, confirmed
  • Status — ✅ Fixed: commit message refilled greedily at 72 columns with Python's textwrap; LC_ALL=en_US.UTF-8 grep -n '.\{73,\}' prints nothing on e53efd9

F2 🟢 Low Priority - The mention case's line is 226 columns and writes the MRI twice ✅ Fixed

  • Document — test/test-teams-copy.sh
  • Location — lines 86–88
  • Issue — Line 88 is 226 columns, the longest in any of the repository's test scripts, and the string can be split, so the exemption for unsplittable strings does not apply. The same MRI literal appears on lines 86 and 88.
  • Suggestion — Set mri=8:orgid:00000000-0000-0000-0000-000000000000 once, use it in the printf that writes the people file and build the expected markup in a variable across lines.
  • Recommendation — Implement — brings the line under 100 columns and removes the duplication
  • Verification — awk 'length > 100' test/test-teams-copy.sh: line 88, 226 columns, confirmed
  • Status — ✅ Fixed: a mention() helper builds the markup from mri and readonly_tag, set once; awk 'length > 100' finds no line

Consistency

F3 🟢 Low Priority - The .ruff.toml comment says "Matches" where the sibling lint configs use the imperative ✅ Fixed

  • Document — .ruff.toml
  • Location — line 1
  • Issue — .yaml-lint.yml says "Match the 100 columns this repo wraps at everywhere else" and .markdown-lint.yml says "Wrap prose at 100 columns"; the new file opens "Matches the repository's 100-column limit."
  • Suggestion — Open with "Match the repository's 100-column limit".
  • Recommendation — Implement — one word aligns the three configs
  • Status — ✅ Fixed: the comment now reads "Match the repository's 100-column limit"

Accuracy

F4 🟡 Medium Priority - "ruff format would rewrite much of both files" overstates its effect ✅ Fixed

  • Document — commit message
  • Location — commit paragraph 2; PR Summary, last bullet
  • Issue — ruff format --diff (0.15.17) removes 44 of teams-copy's 233 lines, all quote-style changes, but only 10 of unfilled-blocks.py's 388, where it explodes hand-wrapped conditions and tuples one item per line. "Much of both files" is false for the second, and the real reason to keep the formatter off is that its style conflicts with the scripts' hand-wrapped style, not the volume.
  • Suggestion — Reword both texts to the measured effect, e.g. "ruff format would restyle both scripts, requoting all of teams-copy's strings and splitting hand-wrapped conditions one item per line".
  • Recommendation — Implement — the claim is checkable and wrong for one file, and the commit message is permanent once merged
  • Concurred by — code-best-practices-reviewer, on the same two texts
  • Verification — uvx ruff@0.15.17 format --no-cache --diff <file> | grep -c '^-[^-]': 44 and 10, confirmed
  • Status — ✅ Fixed: the commit message and PR body now say ruff format would restyle both scripts, requoting teams-copy's strings and re-wrapping the detector's hand-wrapped conditions

F5 🟢 Low Priority - The ticked mutation item names the wrong part of the spacer rule ✅ Fixed

  • Document — PR body
  • Location — Test plan, item 2
  • Issue — The item says "with the spacer rule removed, 3 cases fail". The mutation run replaced if not (is_list(previous) or is_list(block)) with if True, which removes the list exception to the spacer rule; removing the spacer itself fails 2 different cases.
  • Suggestion — "…with the list exception to the spacer rule removed, 3 cases fail".
  • Recommendation — Implement — a ticked item's evidence should name exactly what was run
  • Verification — The orchestrator's own mutation command in this session used sed 's/if not (is_list(previous) or is_list(block)):/if True:/', confirmed
  • Status — ✅ Fixed: the PR's mutation item now lists each mutant that was run

F6 ⚖️ Decision - How to settle the open CI item in the PR's test plan ✅ Fixed

  • Document — PR body
  • Location — Test plan, item 5 ("CI's super-linter run lists teams-copy among the files it hands to Ruff")
  • Issue — At its default log level super-linter lists no per-linter files, and the "Found Python script without extension" message drops to debug inside GNU parallel, so CI run 40123162318 on ef29aa6 shows only "Successfully linted PYTHON_RUFF". The routing is sound on source reading — CheckFileType in lib/functions/buildFileList.sh matches *"Python script"* and adds the file to the Ruff array, and file --brief reports teams-copy as a Python script — but no CI output yet shows teams-copy reaching Ruff, and the item as written can never be ticked.
  • Options
    • (a) Reword to the source evidence and tick it. No further work, but the evidence is source reading rather than a run.
    • (b) Re-run CI with debug logging (gh run rerun 40123162318 --repo mattmenefee/dotfiles --debug) and grep for without extension: .*teams-copy, then reword the item to that evidence. No commit, but it is unverified whether RUNNER_DEBUG reaches super-linter's container.
    • (c) Push a throwaway 101-column line into teams-copy, confirm PYTHON_RUFF fails, then revert. Proves the routing end to end, at the cost of two commits on the branch (squashable).
    • (d) Leave the box open. No cost now, but /ship-it holds it open indefinitely.
  • Recommended — (a) per documentation-expert, since the source and file output fully determine the routing; code-best-practices-reviewer recommends settling it empirically, (b) then (c), because it is the pull request's central "no extra step" claim.
  • Divergence — documentation-expert filed it as a ⚖️ recommending (a); code-best-practices-reviewer filed it as 🟡 Medium, Implement, recommending a debug re-run or a throwaway commit.
  • Status — ✅ Fixed: ruled (b), "Debug re-run". gh run rerun 40123162318 --debug reached super-linter's container and settled the item in the negative: Ubuntu's file reports teams-copy as "HTML document, Unicode text, UTF-8 text", CheckFileType logs "Failed to get file type" and the run logs "There are 1 items to lint for PYTHON_RUFF: …/unfilled-blocks.py". The defect this exposes is F25.

Correctness

F7 🟡 Medium Priority - The link-address case passes with link addresses unprotected ✅ Fixed

  • Document — test/test-teams-copy.sh
  • Location — lines 80–81, "asterisks in a link address are not italics"
  • Issue — In a*b*c each * borders a word character, so ITALIC's lookarounds already reject it. Removing keep() from the <a href> opening in inline() still passes the suite.
  • Suggestion — Use 'See [docs](https://x.y/*a*/b) now\n', expecting '<p>See <a href="https://x.y/*a*/b">docs</a> now</p>'; the mutant renders href="https://x.y/<i>a</i>/b".
  • Recommendation — Implement — the case gives false confidence today
  • Verification — Mutant in a scratch copy: a*b*c renders unchanged, *a*/b renders <i>a</i> inside the address, confirmed
  • Status — ✅ Fixed: the case uses https://x.y/*a*/b; the link-protection mutant now fails it

F8 🟡 Medium Priority - The empty-draft case inherits a malformed people file, and lazy loading is never tested by name ✅ Fixed

  • Document — test/test-teams-copy.sh
  • Location — lines 92–98
  • Issue — Line 92 leaves malformed JSON in the people file, so "an empty draft stops" runs against it. Making the script load the file eagerly fails only that unrelated case, so the docstring's "Without the file, everything but mentions still works" is covered by case order alone.
  • Suggestion — After the malformed-file case, add expect "a draft without mentions ignores a malformed people file" '<p>Plain</p>' 'Plain\n', then rm -f "${people}".
  • Recommendation — Implement — tests the lazy-load contract by name and removes the order dependence
  • Status — ✅ Fixed: added "a draft without mentions ignores a malformed people file" and rm -f -- "${people}" after it; the eager-load mutant fails that case

F25 ⚖️ Decision - Super-linter never hands teams-copy to Ruff in CI ✅ Fixed

  • Document — .github/workflows/ci.yml
  • Location — the Run the linters step, VALIDATE_PYTHON_RUFF
  • Issue — Super-linter classifies an extensionless file by file --brief. macOS's file calls teams-copy a Python script, but the libmagic in super-linter's image reports "HTML document", most likely from the HTML string literals in the script, so CheckFileType gives up and the file joins no Python array. Ruff lints only unfilled-blocks.py, and the pull request's central claim — teams-copy is linted alongside it with no extra step — is false. Found by F6's debug re-run.
  • Options
    • (a) Add a CI step that runs Ruff on teams-copy by path, e.g. pipx run ruff==0.15.17 check home/.local/bin/teams-copy, reading the same .ruff.toml. Covers the file now; the pinned version must be bumped by hand to match super-linter's, or the two can disagree on default rules.
    • (b) Drop teams-copy from the linting claim and lint only .py files, correcting the commit message and PR body. No new machinery, but the untested-by-lint file stays the one with no extension.
    • (c) Rename the script to teams-copy.py so super-linter routes it by extension. Fully automatic, but it renames the command on PATH, and the user-wide CLAUDE.md and the docstring name teams-copy.
  • Recommended — (a), because teams-copy is the larger and more often changed script and a one-line step with a pinned version mirrors how this workflow already gives zsh files the syntax check super-linter cannot.
  • Verification — gh run view 40123162318 --repo mattmenefee/dotfiles --log after the debug re-run: "Detected file type for …/teams-copy: HTML document", "Failed to get file type for …/teams-copy" and "There are 1 items to lint for PYTHON_RUFF: …/unfilled-blocks.py", confirmed
  • Status — ✅ Fixed: ruled (a), "Explicit Ruff step". ci.yml gains "Lint teams-copy with Ruff", which runs the Ruff inside super-linter's own image on teams-copy, with the image tag read from the super-linter step's uses: line so the versions cannot drift (replacing a first pipx run --spec ruff==0.15.17 version that Dependabot could not keep in step); CI run 40125954265 on 197f2fd reported "All checks passed!"

Test Coverage

F9 🟡 Medium Priority - Reading a draft from a path is never tested, nor a missing file ✅ Fixed

  • Document — test/test-teams-copy.sh
  • Location — run(), line 31, and the Input section
  • Issue — Every case pipes the draft through stdin, yet teams-copy draft.md is the main documented usage. Replacing read_draft's path branch with sys.stdin.read() survives the suite, and the OSError branch has no case.
  • Suggestion — Pass extra arguments through run() as the sibling expect_run does with "${@:5}", then add "a draft is read from a path" and "a draft that does not exist stops" (No such file or directory).
  • Recommendation — Implement — covers the main entry point and an error branch at little cost
  • Status — ✅ Fixed: run() passes extra arguments through; "a draft is read from a path" and "a draft that does not exist stops" added, and the path-ignored mutant fails both

F10 🟡 Medium Priority - The + and * bullets, the 1) marker and heading levels 2–6 are untested ✅ Fixed

  • Document — test/test-teams-copy.sh
  • Location — Lists section (lines 63–71) and the heading case (lines 58–59)
  • Issue — Dropping + from [-*+], dropping ) from [.)] and narrowing #{1,6} to #{1} each survive the suite; * item as a list and ####### falling back to a paragraph are untested.
  • Suggestion — Add cases for + a\n* b\n, 1) a\n2) b\n, ## T\n\n###### U\n and ####### T\n.
  • Recommendation — Implement — each is in the docstring's Supported list or LIST_MARKERS
  • Status — ✅ Fixed: four cases added; the +, ) and #{1} mutants each fail one

F11 🟢 Low Priority - Unescaping & in link addresses and mention names is untested ✅ Fixed

  • Document — test/test-teams-copy.sh
  • Location — Inline markup and Mentions sections
  • Issue — Dropping html.unescape in link() double-escapes ?a=1&b=2 to &amp;amp;; dropping it in mention() makes @[A&B] fail its lookup. Both survive.
  • Suggestion — Add expect "an & in a link address is escaped once" '<p><a href="https://x.y/?a=1&amp;b=2">l</a></p>' '[l](https://x.y/?a=1&b=2)\n' and an "A&B" key with a case expecting aria-label="A&amp;B">A&amp;B</mention>.
  • Recommendation — Implement — double escaping is the classic failure in an escape-first design
  • Status — ✅ Fixed: link-address and A&B mention cases added; both unescape mutants fail

F12 🟢 Low Priority - Mentions inside code spans and NUL stripping are untested ✅ Fixed

  • Document — test/test-teams-copy.sh
  • Location — Inline markup section
  • Issue — Moving the mention substitution ahead of code-span protection makes `@[Sam]` abort instead of rendering <code>@[Sam]</code>; removing text.replace('\0', '') lets a NUL-0-NUL draft paste in a copy of the first code span. Both survive.
  • Suggestion — Add expect "a mention inside a code span stays literal" '<p><code>@[Sam]</code></p>' '`@[Sam]`\n' and, optionally, expect "a NUL in the draft cannot inject kept markup" '<p><code>x</code> 0</p>' '`x` \00000\0000\n'.
  • Recommendation — Implement the code-span case; Defer the NUL case, which guards against a hostile draft rather than a likely one
  • Status — ✅ Fixed: both cases added, NUL included; the mention-before-code-span and NUL-strip mutants each fail one

F13 🟢 Low Priority - Spaced asterisks are not tested to stay literal ✅ Fixed

  • Document — test/test-teams-copy.sh
  • Location — "bold and italics", lines 74–75
  • Issue — Removing ITALIC's (?=\S) and (?<=\S) guards survives; 2 * 3 * 4 would render 2 <i> 3 </i> 4.
  • Suggestion — Add expect "spaced asterisks are not italics" '<p>2 * 3 * 4</p>' '2 * 3 * 4\n'.
  • Recommendation — Implement — one line for a regex that is easy to break
  • Status — ✅ Fixed: "spaced asterisks are not italics" added; the guard-removal mutant fails it

F14 🟢 Low Priority - The people file's shape check is untested ✅ Fixed

  • Document — test/test-teams-copy.sh
  • Location — Mentions section, after line 94
  • Issue — Replacing the isinstance check with if False: survives; a list or a numeric id then crashes with a traceback instead of "must be a JSON object".
  • Suggestion — Add printf '{"Alex": 1}\n' >"${people}" and expect_error "a people file with a non-string id stops" 'must be a JSON object' 'Hi @[Alex]\n'.
  • Recommendation — Implement — the only error path with no case
  • Status — ✅ Fixed: "a people file with a non-string id stops" added; the if False: mutant fails it

F15 🟢 Low Priority - --help is untested ⏸️ Deferred

  • Document — test/test-teams-copy.sh
  • Location — whole file
  • Issue — The user-wide CLAUDE.md sends agents to teams-copy --help for the supported Markdown, and nothing checks that it exits 0 and prints the Supported list.
  • Suggestion — Add one assertion that --help output contains ^Supported:, incrementing n.
  • Recommendation — Defer — breaking it is unlikely; add it when the CLI options change
  • Status — ⏸️ Deferred: ruled Defer by the user — add a --help case when the CLI options change

F16 ℹ️ Observation - The stdin-is-a-terminal error cannot reasonably be tested in CI

  • Document — test/test-teams-copy.sh
  • Location — parse_args's sys.stdin.isatty() branch
  • Issue — Testing it needs a pseudo-terminal, and script takes different arguments on BSD and util-linux; the branch is argparse's own parser.error, exit 2, so a portable case would cost more than it protects.

Clarity and Structure

F17 🟢 Low Priority - expect neither checks the exit status nor shows stderr on failure ✅ Fixed

  • Document — test/test-teams-copy.sh
  • Location — expect(), lines 35–40
  • Issue — A crash shows in CI as an empty actual:; the traceback goes to ${dir}/stderr, which the EXIT trap deletes. A run that prints correct HTML but exits nonzero would pass.
  • Suggestion — Capture rc=$? and require 0, as expect_run does in the sibling test, and print the stderr file on any mismatch.
  • Recommendation — Implement — makes CI failures diagnosable without a local rerun
  • Status — ✅ Fixed: expect requires exit status 0 and prints the stderr file on any mismatch; a crash mutant now shows its traceback in the failure

F18 🟢 Low Priority - The header's "control which one exists" misdescribes the mention cases ✅ Fixed

  • Document — test/test-teams-copy.sh
  • Location — lines 8–9
  • Issue — "which one exists" suggests a choice among several people files; the cases decide whether the one file exists and what it holds.
  • Suggestion — "…so the real people file is never read and the mention cases decide whether one exists and what it holds", then refill the comment.
  • Recommendation — Implement — one-line wording fix
  • Concurred by — documentation-expert, with the same rewording
  • Status — ✅ Fixed: the header now says the mention cases "decide whether one exists and what it holds"

F19 🟢 Low Priority - The E501 explanation sits above line-length rather than the line it explains ✅ Fixed

  • Document — .ruff.toml
  • Location — lines 1–2
  • Issue — The second sentence justifies extend-select = ["E501", …] four lines further down. The first sentence alone is a single phrase and would take no final period.
  • Suggestion — Split it: a phrase above line-length, and "Ruff's default rules leave out line length, so E501 is selected explicitly" directly above extend-select.
  • Recommendation — Implement — keeps each reason beside its setting
  • Status — ✅ Fixed: split into a phrase above line-length and the E501 reason directly above extend-select

F20 🟢 Low Priority - "teams-copy was never run at all" reads as absolute ✅ Fixed

  • Document — commit message
  • Location — paragraph 1, last sentence
  • Issue — The paragraph is about CI, but out of context the sentence says the tool never ran anywhere.
  • Suggestion — "teams-copy never ran in CI at all".
  • Recommendation — Implement — a few words in permanent text
  • Concurred by — code-best-practices-reviewer, with the same rewording
  • Status — ✅ Fixed: the commit message now says "teams-copy never ran in CI at all"

F21 🟢 Low Priority - The reason for the top-level test directory leaves out its premise ✅ Fixed

  • Document — commit message
  • Location — last sentence of the commit body; the paragraph after the PR's Summary bullets
  • Issue — "because homesick links everything under home/.local/bin onto PATH" skips that the obvious home for the test is beside teams-copy.
  • Suggestion — "The test sits in a top-level test directory rather than beside teams-copy, because homesick links each file in home/.local/bin into ~/.local/bin, which is on PATH."
  • Recommendation — Implement — states the premise the reason depends on
  • Status — ✅ Fixed: the commit message and PR body now say the test sits "rather than beside teams-copy, because homesick links each file in home/.local/bin into ~/.local/bin, which is on PATH"

F22 ℹ️ Observation - A backslash in a draft must be doubled

  • Document — test/test-teams-copy.sh
  • Location — run(), line 31
  • Issue — Drafts pass through printf '%b', so a Markdown backslash is written \\ and \c silently truncates the draft. No current draft holds one, and the expect comment already says "with printf %b escapes", as the sibling test does.

F23 ℹ️ Observation - Ruff's cache now lands in the CI workspace

  • Document — .ruff.toml
  • Location — whole file
  • Issue — A root .ruff.toml replaces super-linter's bundled template, which set cache-dir = "/tmp/.ruff_cache", so Ruff writes .ruff_cache beside the config, in CI and in a local run without --no-cache. It is harmless: the cache carries its own * .gitignore, and super-linter builds its file list from git before linting.

Staleness

F24 💡 Observation (optional action) - Nothing in teams-copy points to its new test

  • Document — home/.local/bin/teams-copy
  • Location — below the imports
  • Issue — No README, CLAUDE.md or skill lists CI's test steps, so nothing is stale, but the test sits in an unexpected top-level directory. Optionally add # Tested by test/test-teams-copy.sh, which CI runs below the imports, outside the docstring that is the published --help text; CI catches a broken converter regardless.

Consolidated Summary

Finding Type Category Description Document Location Recommendation Group Status
F1 🟢 Low Formatting Commit body not refilled to 72 columns commit message Paragraphs 1–3 Implement G2 ✅
F2 🟢 Low Formatting Mention line is 226 columns, MRI written twice test/test-teams-copy.sh Lines 86–88 Implement G5 ✅
F3 🟢 Low Consistency "Matches" where sibling configs use the imperative .ruff.toml Line 1 Implement G6 ✅
F4 🟡 Medium Accuracy ruff format effect overstated commit message Paragraph 2; PR Summary Implement G2 ✅
F5 🟢 Low Accuracy Mutation item names the wrong part of the spacer rule PR body Test plan, item 2 Implement G2 ✅
F6 ⚖️ Decision Accuracy How to settle the open CI test-plan item PR body Test plan, item 5 Options (a) / (b) G1 ✅
F7 🟡 Medium Correctness Link-address case passes with protection removed test/test-teams-copy.sh Lines 80–81 Implement G3 ✅
F8 🟡 Medium Correctness Empty-draft case inherits a malformed people file test/test-teams-copy.sh Lines 92–98 Implement G3 ✅
F9 🟡 Medium Test Coverage Draft-by-path and missing file untested test/test-teams-copy.sh run(), Input Implement G4 ✅
F10 🟡 Medium Test Coverage +/* bullets, 1) and h2–h6 untested test/test-teams-copy.sh Lists, headings Implement G4 ✅
F11 🟢 Low Test Coverage & unescaping in links and mentions untested test/test-teams-copy.sh Inline, Mentions Implement G4 ✅
F12 🟢 Low Test Coverage Mentions in code spans and NUL stripping untested test/test-teams-copy.sh Inline markup Implement G4 ✅
F13 🟢 Low Test Coverage Spaced asterisks not tested as literal test/test-teams-copy.sh Lines 74–75 Implement G4 ✅
F14 🟢 Low Test Coverage People file shape check untested test/test-teams-copy.sh Mentions Implement G4 ✅
F15 🟢 Low Test Coverage --help untested test/test-teams-copy.sh Whole file Defer — ⏸️
F16 ℹ️ Observation Test Coverage TTY error branch impractical to test test/test-teams-copy.sh parse_args — — —
F17 🟢 Low Clarity and Structure expect ignores exit status and hides stderr test/test-teams-copy.sh expect() Implement G5 ✅
F18 🟢 Low Clarity and Structure Header misdescribes the mention cases test/test-teams-copy.sh Lines 8–9 Implement G5 ✅
F19 🟢 Low Clarity and Structure E501 reason sits away from extend-select .ruff.toml Lines 1–2 Implement G6 ✅
F20 🟢 Low Clarity and Structure "never run at all" reads as absolute commit message Paragraph 1 Implement G2 ✅
F21 🟢 Low Clarity and Structure test/ reason leaves out its premise commit message Last sentence; PR body Implement G2 ✅
F22 ℹ️ Observation Clarity and Structure Backslashes in drafts must be doubled test/test-teams-copy.sh run() — — —
F23 ℹ️ Observation Clarity and Structure Ruff's cache lands in the workspace .ruff.toml Whole file — — —
F24 💡 Observation Staleness No pointer from teams-copy to its test home/.local/bin/teams-copy Below imports — — —
F25 ⚖️ Decision Correctness Super-linter never hands teams-copy to Ruff .github/workflows/ci.yml Run the linters Options (a) G7 ✅

Pre-Merge Checklist

G1 ✅ — Settle how the CI item is evidenced

Closed by the debug re-run, whose answer opened G7.

  • ✅ F6 - How to settle the open CI test-plan item (ruled (b) — the debug re-run showed Ruff never gets teams-copy)

G7 ✅ — Get teams-copy linted, or stop claiming it is

Closed by the explicit Ruff step, which CI ran on 197f2fd.

  • ✅ F25 - Super-linter never hands teams-copy to Ruff (ruled (a) — explicit Ruff step)

G3 ✅ — Make the weak and order-dependent cases test what they name

Closed: each case now fails under the mutant it names.

  • ✅ F7 - Use a link address whose asterisks the italics rule would otherwise take
  • ✅ F8 - Add a lazy-load case and reset the people file after the malformed one

G4 ✅ — Fill the coverage gaps

Closed: every case added, each verified against its mutant.

  • ✅ F9 - Pass arguments through run() and test drafts read from a path
  • ✅ F10 - Test +/* bullets, 1) and heading levels 2–6
  • ✅ F11 - Test & unescaping in link addresses and mention names
  • ✅ F12 - Test a mention inside a code span (NUL case optional)
  • ✅ F13 - Test that spaced asterisks stay literal
  • ✅ F14 - Test the people file's shape check

G5 ✅ — Tidy the test harness and its comments

Closed: harness, long line and header fixed in the same edit.

  • ✅ F17 - Check the exit status in expect and show stderr on failure
  • ✅ F2 - Split the mention line and set the MRI once
  • ✅ F18 - Reword the header's description of the mention cases

G6 ✅ — Tidy the .ruff.toml comments

Closed: both comments rewritten.

  • ✅ F3 - Use the imperative "Match", as the sibling configs do
  • ✅ F19 - Put the E501 reason above extend-select

G2 ✅ — Correct the commit message and PR body

Closed by the amended commit e53efd9 and the edited PR body.

  • ✅ F4 - Restate what ruff format would change
  • ✅ F5 - Name the list exception as the mutation that was run
  • ✅ F20 - Say teams-copy never ran in CI
  • ✅ F21 - State why the test is not beside teams-copy
  • ✅ F1 - Refill the commit body to 72 columns

Not recommended for this revision

  • ⏸️ F15 - Test --help (deferred — add it when the CLI options change)

The repository now carries two Python scripts, the /reflow block
detector and teams-copy, and CI checked neither as Python. The detector
at least had a fixture test; teams-copy never ran in CI at all.

Super-linter now runs Ruff's linter, which picks up the .py detector. It
does not reach teams-copy: super-linter routes an extensionless file by
what `file` reports, and the `file` in its image classifies the script
as an HTML document, so it joins no Python linter's list. A separate
step runs the Ruff inside super-linter's own image on teams-copy by
path, with the image tag read from the super-linter step, so the two
runs share one Ruff version and its default rules. A root .ruff.toml,
which both runs read, sets the repository's 100-column limit and adds
E501, which Ruff's default rules leave out, plus the W and I rules. Both
scripts pass as they stand.

The formatter and mypy stay off. `ruff format` would restyle both
scripts, requoting teams-copy's strings and re-wrapping the detector's
hand-wrapped conditions, and neither script has the annotations a type
checker needs.

A fixture test for teams-copy runs it with --print-html, which skips the
macOS clipboard, so the Markdown conversion is exercised on
ubuntu-latest. It covers headings, every list marker and how lists group
and number, spacer placement, inline markup and escaping, code spans and
link addresses, mentions and each people-file error, and drafts read
from stdin or a path. HOME points at a private directory so the real
people file is never read. The test sits in a top-level test directory
rather than beside teams-copy, because homesick links each file in
home/.local/bin into ~/.local/bin, which is on PATH.
The repository's new .ruff.toml sets a 100-column limit, but the /reflow
block detector was hand-wrapped, and many of its test's cases broke
across two lines where one would fit.

The detector is now formatted by Ruff 0.15.17 under that config, which
only lays out code differently: the parsed syntax tree is unchanged.
Each test case that fits in 100 columns now sits on one line; a case
that would run past the limit stays split.
@mattmenefee
mattmenefee marked this pull request as ready for review October 24, 2026 20:07
@mattmenefee
mattmenefee merged commit 6729cdb into main Oct 24, 2026
10 checks passed
@mattmenefee
mattmenefee deleted the lint-and-test-python branch October 24, 2026 20:10
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.

1 participant