Repository navigation
Conversation
|
@claude review |
There was a problem hiding this comment.
Beyond the inline findings, I also checked whether binary (non-UTF-8) skill files would be corrupted by writeVerifiedFile's Buffer.from(content, "utf8") — the pinned OpenAPI spec defines file content as a plain string with no base64/binary encoding, so this looks like an intentional scope limit for text-based skills rather than a bug.
Extended reasoning...
The diff adds a new skills install/update CLI feature with filesystem writes (atomic directory staging/swap), a lockfile, and new HTTP calls with zod validation against a hosted API — security-relevant in the sense of untrusted file paths and checksums, though path traversal itself is guarded. Four confirmed findings (case-insensitive path collision, nested-directory --force crash, misleading exit code on malformed JSON, and a skill literally named "help" being unreachable) already exist as inline comments, and the hunt's exit reason was a budget bound rather than running dry, so approval is not warranted here.
Findings marked 🟡 are optional suggestions and need no follow-up push.
|
@claude review |
There was a problem hiding this comment.
I reviewed this PR; the inline findings above cover the issues I found. Beyond those, I also checked the rollback path in replaceDirectory (src/skills.ts:211-225): if the restore rename after a failed install also fails, the original error still propagates rather than being silently swallowed, so I ruled that out as a separate bug.
Extended reasoning...
This run surfaced four new CONFIRMED findings (lock-file path traversal in targets.ts, an update loop that can abort entirely on one pruned skill version, an overlap-check bypass via --no-lockfile enabling later silent deletion, and an unbounded pagination loop in the tag-fetch path) that will be posted as inline comments; these touch filesystem writes outside the project tree and availability/DoS-adjacent behavior, which is security-sensitive enough to warrant human review. A prior review round from this same system already left four inline threads that were resolved only by the PR author (not independent verification), so those remain effectively open per the review policy. Given the volume and nature of confirmed findings, approval is not appropriate; I additionally verified the one ruled-out candidate (rollback-failure data loss in replaceDirectory) by reading the code and confirmed the original error still surfaces rather than being swallowed.
Findings marked 🟡 are optional suggestions and need no follow-up push.
|
@claude review |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the newly flagged finding, I re-verified the concerns raised in my two earlier reviews against this push. The literal skill name "help" being swallowed by the help-flag check, the nested-directory overlap crash mid-update, lockfile paths escaping the project (absolute/relative/symlink), and the unbounded --tag pagination loop are now fixed in commits 6f36485 and a0e1113.
Extended reasoning...
This push hardens the new skills install/update feature (lockfile confinement via realpath checks, overlap rejection, EXIT_RESPONSE vs EXIT_HTTP separation, bounded tag-list pagination). Reading the diff, I confirmed four previously-raised concerns from my earlier reviews are now fixed in commits 6f36485 and a0e1113. A new finding was confirmed this run, and one earlier concern remains unaddressed in the code despite its thread being marked resolved by the author, so a human should still look before merge.
Adds a proof of concept for installing and updating Langfuse-hosted skills through the CLI. Users can select a skill by name, label, or immutable version, or install all skills with a tag. The PR remains a draft for evaluating the CLI workflow.
Behavior
langfuse skills install <name[@selector]>defaults to theproductionlabel and.agents/skills. Supports--tag,--label,--version,--directory,--no-lockfile,--force, and--json.langfuse skills installrestores exact versions and directories fromlangfuse-skills-lock.json.langfuse skills update [name]follows saved labels while keeping explicit versions pinned. Clean recorded installations can update; local differences require--force.--directorytogether with--no-lockfile.--forceallows replacement without the historical check.--no-lockfileskips lockfile reads/writes and recorded-directory overlap checks.helpcan be installed and updated. Use--helpor-hfor action help.langfuse api skills. Installer modules use shared Zod validation. No release version bump.Try it
With the usual Langfuse key and host configuration, use a disposable directory:
Replace example skill names/tags with ones available in your project.
Validation
Impacted package:
@langfuse/cli.bun test:108 pass,0 fail(local socket access required for HTTP capture tests).pnpm exec tsc --noEmit: exit 0.bun run build:Built native Bun CLI with 7 contracts and 720 operations.helpskill name, response error codes, and checksum failure preservation. Fixtures use mocked API responses and temporary directories; no database seed is needed for these filesystem and HTTP-contract cases.origin/main.git diff --checkflags two trailing-whitespace lines in the checksum-pinned upstream OpenAPI snapshot; retained verbatim.