Skip to content

skills: lucidlint LSP update procedure; placement test for new skills - #59

Merged
omp-harness[bot] merged 10 commits into
mainfrom
skill/lucidlint-lsp-update-and-placement
Oct 31, 2026
Merged

omp-harness[bot] merged 10 commits into
mainfrom
skill/lucidlint-lsp-update-and-placement

Conversation

@omp-harness

@omp-harness omp-harness Bot commented Sep 6, 2026 •

Copy link
Copy Markdown
Contributor

User description

New skill: lucidlint-lsp-update

The update procedure is a tool now, not prose: tools/lucidlint-lsp-update.sh
(installed by make install as lucidlint-lsp-update.sh) downloads the
release asset for the machine's architecture, replaces
~/.local/share/lucidlint/bin/lucidlint, kills the clients still serving the
old rules, and prints what a stdio initialize handshake answers. The skill
names the command and states the served version as the proof; it carries no
shell of its own.

Placement test added to update-skills

New skill decisions now pass: who needs this information, and where will
they be standing when they need it?
Cross-repo operational knowledge
(updating a shared tool's installed component) belongs in a harness skill — a
tool repo's own docs are invisible to client-repo agents, which load their own
repo's context and never another repo's AGENTS.md.

Anti-patterns added for the two failures this PR came from: shell inside a
skill for the agent to execute, and restating a tool's decision as a prose
table.

lucidlint-workflow install section

Install is one pip line pinned to the v0.6.9 commit; the libcst aside and the
old pre-PyPI wording are gone.

Verification

  • make test: 60 docs scanned, 41 skills well-formed, links resolve, 18 tests OK.
  • The updater ran end to end: installed v0.6.9, handshake answered
    "version":"0.6.9".
  • Deployed with make install (symlinks refreshed); restart omp to load the new
    skill — required for any skill change to take effect.

PR Type

Enhancement, Documentation


Description

  • Add lucidlint-lsp-update skill for one-pass LSP updates

  • Update lucidlint-workflow install guidance to release wheels

  • Add placement test to update-skills for cross-repo knowledge

  • Document gotchas: name collision and client restart requirements


Diagram Walkthrough

flowchart LR
  A["New skill: lucidlint-lsp-update"] --> B["Update installed LSP"]
  B --> C["Verify served version"]
  C --> D["Kill running clients"]
  E["Update lucidlint-workflow"] --> F["Release wheels install"]
  G["update-skills placement test"] --> H["Cross-repo knowledge"]
Loading

File Walkthrough

Relevant files
Enhancement
SKILL.md
New skill for lucidlint LSP updates                                           

skills/lucidlint-lsp-update/SKILL.md

  • New skill for updating the installed lucidlint LSP server
  • Provides one-pass update: download release, install binary, kill
    clients
  • Includes served-version verification via initialize handshake
  • Documents gotchas: name collision with lucid-lint and client survival
+56/-0   
SKILL.md
Add placement test for new skills                                               

skills/update-skills/SKILL.md

  • Adds placement test for new skill decisions
  • Test: who needs info and where they'll be standing
  • Cross-repo operational knowledge belongs in harness skills
  • Documents real case: LSP procedure unreadable by client agents
+12/-0   
Documentation
SKILL.md
Update install guidance to release wheels                               

skills/lucidlint-workflow/SKILL.md

  • Replaces pre-PyPI commit pin with release wheel install guidance
  • Adds tag-pin alternative for pip install
  • Links to new lucidlint-lsp-update skill for LSP server updates
+10/-7   

- NEW lucidlint-lsp-update: update the installed LSP in one pass
  (release bundle -> install path -> kill clients) and verify the SERVED
  version with an initialize handshake; warns off the unrelated
  '~/.local/bin/lucid-lint' prose linter whose name collides.
- update-skills: the placement test — who needs the information, and
  where will they be standing? Cross-repo operational knowledge belongs
  in a harness skill; a tool repo's own docs are invisible to
  client-repo agents. (A complete LSP-update procedure was written into
  the tool repo's docs, unreadable by exactly the agents who needed it.)
- lucidlint-workflow: install guidance no longer pins a pre-PyPI commit;
  points at the release wheels and the LSP-update skill.
@github-actions

github-actions Bot commented Sep 6, 2026 •

Copy link
Copy Markdown

PR Reviewer Guide 🔍

(Review updated until commit b8207bf)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Possible Issue

The timeout 3 command may not be available on all systems (e.g., macOS without coreutils). If timeout is missing, the handshake command fails with a "command not found" error, misleading the user into thinking the LSP is broken. Consider documenting the dependency or using a portable alternative.

| timeout 3 "$BIN" --lsp | grep -o '"version":"[^"]*"' || true)"
✅ Resolved findings

skills/lucidlint-workflow/SKILL.md:24-30

Compliance Issue

The multi-line code block (lines 27-30) is a runnable script that should be moved to an examples/ file and referenced, per docs/writing-documentation.md: "Code in code files, not inline in docs. A doc that contains a multi-line code block (config file, script, workflow) should move that code to a separate file and reference it."

Makefile:47

Compliance Issue

The PR does not confirm that make test passes. Changes to skills or standards must keep the repo self-check green and mention that the change needs make install + omp restart to take effect. The PR description mentions the install and restart but omits the test verification, making it impossible to verify compliance with the requirement.

tools/lucidlint-lsp-update.sh:37

Possible Issue

The initialize handshake sends an empty params object. Some LSP servers require processId, rootUri, or capabilities in the initialize request and may reject or hang on an empty params. If the server rejects the handshake, the version check fails even though the update succeeded. Consider sending a minimal valid params object.

skills/lucidlint-lsp-update/SKILL.md:38

Possible Issue

The initialize handshake sends an empty params object. Some LSP servers require processId, rootUri, or capabilities in the initialize request and may reject or hang on an empty params. If the server rejects the handshake, the version check fails even though the update succeeded. Consider sending a minimal valid params object.

skills/lucidlint-lsp-update/SKILL.md:41

Possible Issue

The timeout 3 command may not be available on all systems (e.g., macOS without coreutils). If timeout is missing, the handshake command fails with a "command not found" error, misleading the user into thinking the LSP is broken. Consider documenting the dependency or using a portable alternative.

skills/lucidlint-lsp-update/SKILL.md:26-27

Possible Issue

The install -m 755 command on line 26-27 does not create parent directories. If ~/.local/share/lucidlint/bin/ does not exist, the install fails silently or with an error. The skill should ensure the directory exists (e.g., mkdir -p ~/.local/share/lucidlint/bin) before running install.

skills/lucidlint-lsp-update/SKILL.md:27-28

Possible Issue The pkill -f 'lucidlint --lsp' pattern can match the shell command itself or other processes whose command line contains that substring, potentially killing unrelated processes. A more precise pattern (e.g., matching the full binary path) would reduce the risk of collateral kills.

Comment thread skills/lucidlint-lsp-update/SKILL.md Resolved
Comment thread skills/lucidlint-lsp-update/SKILL.md Resolved
Comment thread skills/lucidlint-lsp-update/SKILL.md Resolved
Embedding a version in a skill rots silently (the lucidlint-workflow
install section pinned a pre-PyPI commit and nothing detected it).
lucidlint-workflow now resolves the latest release wheel at run time;
lucidlint-lsp-update derives VER from the latest release tag by default
(pin only to repeat an install); update-skills gains the anti-pattern:
never embed per-release facts — teach the lookup.
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown

Standalone PR Review

PR-Agent could not safely update the persistent review. This standalone result will not replace the canonical review.

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Possible Issue

The pkill -f 'lucidlint --lsp' command may kill unrelated processes if the pattern matches other command lines containing that substring (e.g., a shell script or a different tool invoked with similar arguments). The skill should scope the kill to the exact binary path or use a more specific pattern to avoid terminating unintended processes.

pkill -f 'lucidlint --lsp'   # clients keep serving the old rules until
                             # killed; the editor/omp respawns on demand
Possible Issue

The verification handshake uses timeout 3 and pipes to grep -o '"version":"[^"]*"'. If the server takes longer than 3 seconds to respond (e.g., on a slow machine or under load), the command will exit with a timeout and produce no output, making the verification appear to fail even though the update succeeded. Consider increasing the timeout or handling the timeout case explicitly.

INIT='{"jsonrpc":"2.0","id":1,"method":"initialize","params":{}}'
printf 'Content-Length: %d\r\n\r\n%s' "${#INIT}" "$INIT" \
  | timeout 3 ~/.local/share/lucidlint/bin/lucidlint --lsp \
  | grep -o '"version":"[^"]*"'
# expect the installed release, e.g. "version":"v0.5.0"
Possible Issue

The install command uses pip install ./lucidlint-*-py3-none-linux_x86_64.whl with a glob. If multiple wheel files matching the pattern exist in the current directory (e.g., from a previous download), pip will attempt to install all of them, which may fail or install an unintended version. The skill should either remove the glob after download or use a specific filename.

gh release download --repo ashbywinch/lucidlint \
  --pattern 'lucidlint-*-py3-none-linux_x86_64.whl'
pip install ./lucidlint-*-py3-none-linux_x86_64.whl

Comment thread skills/lucidlint-lsp-update/SKILL.md Resolved
Comment thread skills/lucidlint-lsp-update/SKILL.md Resolved
Comment thread skills/update-skills/SKILL.md Resolved
…te length

Review findings. pkill -f matched any cmdline containing the string (including
the invoking shell); the anchored full-path pattern matches only the LSP
servers. The initialize handshake omitted the required Content-Type header and
sized the body with ${#INIT} (characters) instead of bytes — wrong for any
multi-byte payload. update-skills gains the concrete run-time-lookup example
the reviewer asked for.
Comment thread skills/update-skills/SKILL.md Resolved
Comment thread skills/update-skills/SKILL.md Resolved
@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit 6136b90

Comment thread skills/lucidlint-workflow/SKILL.md Resolved
Comment thread skills/lucidlint-lsp-update/SKILL.md Resolved
@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit 1e60d73

Comment thread skills/lucidlint-lsp-update/SKILL.md Resolved
@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit 3e87c18

@github-actions

Copy link
Copy Markdown

PR Code Suggestions ✨

No code suggestions found for the PR.

@omp-harness

omp-harness Bot commented Oct 31, 2026

Copy link
Copy Markdown
Contributor Author

Addressing the three ACTIVE findings in the persistent review on 3e87c18 (two now fixed by ea2c57a, one declined with evidence):

initialize handshake sent empty params — fixed in ea2c57a. tools/lucidlint-lsp-update.sh:37-39 now sends {"processId":null,"rootUri":null,"capabilities":{}}. Verified against the real server: the updater installed v0.6.9 and the handshake answered "version":"0.6.9".

timeout 3 may be missing on macOS — declined as out of scope. The script resolves a lucidlint-$VER-x86_64-unknown-linux-musl.tar.gz asset and exits with error: no lucidlint release asset for architecture … for anything else (:17-21), so a macOS host never reaches the handshake step; timeout ships with coreutils on every Linux host the script accepts.

PR does not confirm make test / omits the install+restart step — the description now carries a Verification section with the make test counts (60 docs, 41 skills, links resolve, 18 tests) and states that make install plus an omp restart are required for the skill change to take effect.

@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit ea2c57a

Comment thread tools/lucidlint-lsp-update.sh Resolved
Comment thread tools/lucidlint-lsp-update.sh Resolved
@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit c92cd2b

Comment thread skills/lucidlint-workflow/SKILL.md Resolved
@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit bb3762d

@github-actions

Copy link
Copy Markdown

PR Code Suggestions ✨

No code suggestions found for the PR.

ashbywinch
ashbywinch previously approved these changes Oct 31, 2026
…pdate-and-placement

# Conflicts:
#	skills/lucidlint-workflow/SKILL.md
@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit b8207bf

@github-actions

Copy link
Copy Markdown

PR Code Suggestions ✨

No code suggestions found for the PR.

@omp-harness
omp-harness Bot merged commit 0938f04 into main Oct 31, 2026
3 checks passed
@omp-harness
omp-harness Bot deleted the skill/lucidlint-lsp-update-and-placement branch October 31, 2026 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant