feat: route skill feedback through Langfuse intake - #76
Conversation
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
… bump - drop the two 'api feedback *' allowed-tools entries so the feedback submit call goes through the normal permission prompt instead of being auto-approved (allowlist stays read-only verbs) - replace the GitHub issue fallback with a prefilled discussion link in Ideas & Improvements; offered alongside intake in the consent step for users who want a public, trackable thread - restore proactive trigger guidance in the references list entry and frontmatter description (offer once on wrong/outdated guidance) - drop GITHUB from required_access (no gh CLI usage remains) and align wording with the shipped API (MCP server tool, correlation ID) - bump manifests to 1.5.0 per semver policy (new capability) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@claude review |
There was a problem hiding this comment.
LGTM — the two issues raised in the earlier review round (the wildcarded feedback allowed-tools entry and the patch-vs-minor version bump) are both resolved: SKILL.md's allowed-tools no longer has a feedback-scoped entry at all, and all three manifests now bump to 1.5.0 in lockstep. The findings from this run are nit-level gaps in the fallback GitHub link and sanitization wording, not blocking issues.
Extended reasoning...
Overview
This PR reworks skills/langfuse/references/skill-feedback.md to route skill feedback through an authenticated Langfuse MCP/CLI/API path (with Docs MCP and GitHub issue/discussion as fallbacks), and bumps all three plugin manifests from 1.4.x to 1.5.0 in lockstep. No SKILL.md changes are present in this diff.
Security risks
The submission flow still gates on an explicit human approval step (step 3: show every field, ask permission) before anything is sent, and explicitly forbids asking users to paste credentials. The one prior critical concern — a wildcarded Bash(npx/bunx langfuse-cli api feedback *) allowed-tools entry that would have auto-approved a write/submit call — is no longer present in the current SKILL.md; the allowed-tools list only contains the pre-existing read-only __schema/--help/list/get patterns, so feedback submission still requires a normal permission prompt. This run's two nit findings (issue-link missing prefill params, no explicit secret-stripping instruction before drafting) are real but low-severity: the former is a UX papercut, the latter is mitigated by the mandatory show-and-approve gate in step 3.
Level of scrutiny
This is instruction-only content with no committed code and no runtime security surface beyond documentation guidance shown to an agent. Given the previously blocking issue (auto-approved write action) is resolved and the version bump now correctly reflects a minor capability change per AGENTS.md, a light-touch review is appropriate here.
Other factors
The version bumps across .claude-plugin, .codex-plugin, and .cursor-plugin are aligned at 1.5.0, satisfying the lockstep requirement. No CODEOWNERS paths are touched by this diff.
| Format the body as markdown with the two field labels as headings. | ||
| 1. **Authenticated Langfuse MCP, CLI, or Public API** — prefer the `submitFeedback` tool on the Langfuse MCP server. If it is unavailable, discover the current feedback operation with the Langfuse CLI schema/help and submit through the authenticated Public API. Do not ask users to paste credentials into chat. | ||
| 2. **Langfuse Docs MCP** — use its unauthenticated `submitFeedback` tool when no authenticated Langfuse interface is available. | ||
| 3. **GitHub issue or discussion** — if no MCP/CLI/API is available, or the user wants a public, trackable thread, provide a prefilled discussion link for the user to submit: `https://github.com/langfuse/skills/discussions/new?category=ideas-improvements&title=<url-encoded title>&body=<url-encoded body>`. Use `https://github.com/langfuse/skills/issues/new` if they prefer an issue. |
There was a problem hiding this comment.
🟡 The prefilled GitHub discussion fallback link includes title=/body= query params, but the issue fallback (https://github.com/langfuse/skills/issues/new) doesn't, even though GitHub's issues/new endpoint supports the same params. A user who picks the issue path over the discussion gets a blank form and has to manually retype the feedback they already drafted and approved.
Extended reasoning...
The bug
Line 33 of skill-feedback.md offers two GitHub fallback links for when no authenticated Langfuse interface is available: a discussion link and an issue link. The discussion link is fully prefilled — https://github.com/langfuse/skills/discussions/new?category=ideas-improvements&title=<url-encoded title>&body=<url-encoded body> — carrying over the exact title/body the user just approved in step 3. The issue link, immediately after, is the bare https://github.com/langfuse/skills/issues/new with no query params at all.
Why this is inconsistent, not intentional
GitHub's issues/new endpoint supports the identical title= and body= prefill query params as discussions/new. There's no technical reason the issue link couldn't carry the same params — the omission reads as an oversight in an otherwise-parallel construction (Use <link> if they prefer an issue), not a deliberate distinction between the two paths.
Why the surrounding flow doesn't prevent the problem
The whole point of this reference's draft-then-approve flow (steps 1-3) is that the user signs off on exact wording once, and every submission path downstream should just deliver that approved content. Two of the three "Submission options" bullets do this correctly (the authenticated MCP/CLI path sends the drafted fields directly; the discussion link prefills them via query params). The issue link silently breaks that guarantee for anyone who happens to prefer issues over discussions, or for repos/orgs where discussions are disabled and issue is the only GitHub option.
Step-by-step proof
- User asks the agent for skill feedback; agent drafts title="Docs example uses removed API" and a body describing the problem, and the user approves it (step 3).
- No authenticated Langfuse MCP/CLI/API is available, and the Docs MCP is also unavailable, so the agent falls to option 3 (GitHub).
- The user says they'd rather file it as an issue than a discussion.
- Following the literal instruction, the agent hands them
https://github.com/langfuse/skills/issues/new— no params. - The user opens the link to a completely blank issue form and must retype the title and body they already wrote and approved, defeating the purpose of drafting the feedback first.
Fix
Append the same encoded params to the issue link, mirroring the discussion link: https://github.com/langfuse/skills/issues/new?title=<url-encoded title>&body=<url-encoded body>.
On severity
This is instruction-only content, nothing crashes, and it only affects the secondary "if they prefer an issue" branch of a fallback path that itself only triggers when no authenticated Langfuse interface is available. A capable agent could plausibly infer by analogy that "prefilled" should apply to both links. That keeps this at nit rather than blocking-severity — worst case is a minor retype inconvenience, not a broken workflow.
| 1. Draft concise feedback using the following structure: | ||
| - **Describe your idea or feedback** (required) | ||
| A clear description of what went wrong or what could be improved. Include: | ||
| - What the user was trying to do (as `goal`) | ||
| - What the skill did vs what was expected | ||
| - Any specific instructions that were incorrect or missing | ||
|
|
||
| ## Feedback Form Structure | ||
| - **What would the ideal outcome look like?** (optional) | ||
| What the correct behavior or guidance should be. | ||
|
|
||
| Draft the feedback using these two fields: | ||
| If feedback targets existing skill, reference in `target`. | ||
| 2. If the user wants a reply, ask them to include an email address in `feedback`; use only an address they explicitly provide. | ||
| 3. Show every submitted field exactly as it will be sent and ask for explicit permission. Do not submit without approval. |
There was a problem hiding this comment.
🟡 The explicit instruction to strip secrets, credentials, and trace payloads before drafting feedback (present in an earlier version of this file) was dropped during the restructure, even though the PR description promises the workflow will 'prohibit secrets, customer data, trace payloads, and unrelated context.' The remaining scoping line and the mandatory show-and-approve step (step 3) mitigate this but rely on the user catching sensitive content themselves rather than the agent proactively stripping it before drafting — worth restoring a one-line reminder, especially since option 3 can post the draft publicly to GitHub.
Extended reasoning...
An earlier draft of this file (git history, commit 7634a5f) contained an explicit step: "Remove secrets, credentials, customer data, trace payloads, and unrelated context." Commit b5b4533 restructured the drafting steps and this sentence was dropped; it is absent from the final version at lines 13-25, which now cover only draft structure (step 1), the optional reply email (step 2), and the show-and-approve permission gate (step 3). Meanwhile the PR description's own summary still promises the workflow will "prohibit secrets, customer data, trace payloads, and unrelated context" — so the shipped instructions no longer match the stated intent of the PR, most plausibly as an accidental casualty of the rewrite rather than a deliberate decision to relax the guardrail.
This matters because Submission options option 3 posts the drafted feedback body publicly as a GitHub discussion or issue, and option 1 sends it to the Langfuse backend. An agent drafting feedback from conversation context — e.g. quoting a failing CLI invocation, an error message, or a trace snippet to explain "what the skill did vs what was expected" — has no explicit instruction telling it to scrub that content for embedded secrets or credentials before it ever reaches the draft.
Concretely walking through it: (1) a user hits a bug where a skill instructed them to run a CLI command against an API endpoint, and that command (as run) included an auth token in a flag or environment reference; (2) the user pastes the failing command into chat while describing the problem; (3) the agent, per step 1, quotes "what the skill did vs what was expected" and includes the command verbatim in the draft to make the report concrete; (4) step 3 shows the drafted fields to the user and asks for permission — but the instruction is only to display the field, not to actively check it for secrets, so a user skimming a familiar-looking command may approve without noticing the embedded token; (5) if option 3 is used, that token is now published publicly on GitHub.
The remaining guardrails do meaningfully reduce, but don't close, this gap. Line 11 ("not... a user's application data") scopes out trace payloads and customer data at a topical level, but doesn't clearly cover a credential incidentally embedded in an otherwise on-topic report about the skill itself. Line 31 ("do not ask users to paste credentials into chat") and line 35 (safe error messages) guard against the agent soliciting credentials or leaking them on failure, not against the agent including something the user already pasted. Step 3's show-and-approve gate keeps a human in the loop, but it is a passive review step, not an instruction for the agent to actively strip sensitive content the way the deleted sentence did — it shifts the entire burden onto the user noticing at review time.
The fix is cheap: restore a short instruction in step 1 (or as a standalone step) telling the agent to strip secrets, credentials, and trace/application-data snippets from the drafted text before presenting it for approval, matching what the PR description already promises.
Given that step 3's mandatory human approval gate does stand between drafting and any submission, and nothing crashes or silently leaks data without a user clicking approve, this is best treated as a content regression rather than a merge-blocking defect.
2 participants
Summary
submitFeedbacktool on the Langfuse MCP server, then the generated public API through langfuse-clighauth neededVerification
Tests added: 0 — instruction-only behavior with no test harness in this repository.