Repository navigation
Triage the open template-feedback issues - #100
Merged
Merged
Conversation
…rm, fixes #93 The parameters=[...] list in @plugin reads as the form order and is not. PluginDescription.retrieve_parameters walks inspect.signature(__init__) and looks each argument name up in the decorator list, so the constructor decides the field order and the decorator list only supplies labels, descriptions and types. Reordering the decorator list therefore has no visible effect, and an entry whose name matches no constructor argument is skipped with no error at all - the field still renders, just without its label or description. Verified against cmem-plugin-base description.py.
PasswordParameterType.from_string turns an empty string into Password(""), and Password defines neither __bool__ nor __len__, so an object wrapping no secret at all is truthy. The skill already says to call .decrypt() only where the value is used, which puts the is-it-set question somewhere else, where only the wrapper is at hand - so the guidance was pointing straight at the trap. The helper takes Password | str because a default value is never passed through from_string; base's own to_string carries the same special case for that reason, and a test constructing the plugin directly hands over a str as well. Verified against cmem-plugin-base parameter/password.py.
…check, fixes #96 validators.url() rejects every scheme that is not a URL, urn: included, so a parameter validated with it refuses a graph the store handles - importing into and querying a urn: named graph works. The resulting message is the generic 'Invalid value for parameter ...', which never says the scheme was the problem. The shipped pattern was checked against the RFC 8141 rules it claims: a namespace identifier of two to thirty-two characters starting alphanumeric and a non-empty namespace specific string, accepting urn:example:data, urn:uuid:... and URN:Example:Data while rejecting urn:a:b, urn:-bad:x, urn:example: and a 33 character identifier.
…y restoring the store, fixes #95 Both halves have cost real time. A fixed-id project left behind by a crashed run made every later run of cmem-plugin-validation error at setup until someone removed it by hand; deleting first with skip_if_missing=True is what cmem-plugin-kafka's make_project already does, and the shipped snippet follows it. The second half is the less obvious one, because snapshot-and-restore reads as the more careful choice. It reverts everything else written to the deployment in between, and integration tests across these projects share one instance - so it undoes other people's work to clean up after itself, and took one suite from about 127 seconds to 22 when replaced with targeted deletion.
#97 The skill named backticks, bold and links, then prescribed four beats without saying how to mark them, so it read as a nudge toward unbroken prose while never deciding the question. Generated projects split on it and a user meets both styles in one workspace. The rule ratifies what the plugins already do rather than inventing a preference. Across 68 documentation blocks in the checked out plugins, 26 use ## headings and 42 are prose, and the two groups differ by length almost exactly where this draws the line: a median of 23.5 non-blank lines with headings against 9.5 without. The twenty line mark also matches the fifteen to twenty lines the same section already calls typical.
#98 The answer existed only in plugin-implementation/SKILL.md, which is rendered for project_type: plugin - so a generic project met the same structurally unanswerable rule with nowhere to read that the template had already decided it, and every agent re-asked a human. The rules file ships to both types. Kept to the class rather than the treatment: the plugin constructor keeps its own section, and this names framework-fixed arity in general, a Click command being the other instance. One correction to the report, which does not change the outcome: the five option example it gives does not actually trigger PLR0913. Ruff's default max-args is 5 and both rules fire above it, so the case starts at six options - checked with ruff 0.16.5 under --isolated --select ALL, which also confirms that making the arguments keyword-only clears PLR0917 and leaves PLR0913.
Two of the new entries used their nested bullet to explain the change rather than to warn about what a template user meets, which is what that level is for per CLAUDE.md. No entry was merged: the three plugin-implementation findings share no background to tell twice, and the convention keeps one report per entry.
The bare 'if' had no body, which is the one snippet in the skill that is not parseable Python. The surrounding @plugin fragments already elide with '...', so it now follows them.
Coverage Report
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Triage of the seven open
template-feedbackissues. Six are accepted, one commit each; #99 is left open for a human decision. Every finding was verified against the source or the tool rather than taken from the report.plugin-implementation: the constructor's argument order decides the form, not the@Pluginlistretrieve_parameters()incmem-plugin-base— it walksinspect.signature(__init__), so the decorator list is only a lookup tableplugin-implementation: an empty secret parameter is still truthyparameter/password.py—from_string("")returnsPassword(""), which defines neither__bool__nor__len__plugin-implementation: a graph IRI may be a URN, sovalidators.url()is the wrong checkvalidators.url()rejectsurn:example:data; the shipped pattern checked against 13 RFC 8141 casesplugin-testing: a fixture deletes before as well as after, and cleanup never means restoring the storecmem-plugin-kafka'smake_project, which the snippet followsplugin-documentation: prose while short,##headings past roughly twenty lines.claude/rules/copier-template.mdnamesPLR0913/PLR0917on a framework-fixed signature as the settled exception--isolated --select ALLTwo further commits are polish: the changelog's nested bullets were pulled back to consequences rather than explanations, and one snippet gained the file's
...elision marker so that every Python block in the skill parses.Why #98 was implemented rather than closed as a re-raise
It reads like a re-raise of #69, which is why it was checked first. It is not. #69 settled how to suppress the rule — a
# noqaon thedef, never anignoreentry — and that decision is untouched. #98 only extends that answer's reach: it lived solely inplugin-implementation/SKILL.md, which is rendered forproject_type: plugin, so a generic project met the same structurally unanswerable rule with nowhere to read that the template had already decided it.One correction to the report, recorded in the commit and on the issue: its five-option Click example does not trigger
PLR0913. Ruff's defaultmax-argsis 5 and both rules fire strictly above it, so the case starts at six options. The general claim holds, and keyword-only arguments do clearPLR0917while leavingPLR0913.What
task checkcould not coverIt renders the six cases and runs their Taskfiles; it never starts an agent, so the shipped skill and rule files are checked by hand. Two things were verified in the rendered cases:
generic-project_dirnow receives the Name PLR0913/PLR0917 as a standing exception in the shared rules, not only in the plugin skill #98 answer in.claude/rules/copier-template.mdwhile still getting no plugin material — only thecopier-updateandtemplate-feedbackskills, and nocorporate-memory.md. That delivery is the entire point of the issue.SKILL.mdfiles carry no.jinjasuffix andcopier.ymlsets no_templates_suffix, so their content is copied verbatim and{1,31}is never read as Jinja.task checkis green across all six cases; the skips in the four plugin cases are the CMEM-gated tests.Still open
#99 — a
cliproject type for Click based tools — is deliberately not in this branch. It is a well-argued proposal rather than a decline, but it is a product decision and out of scope for a triage sweep: a thirdproject_typechanges delivery scope, which by the 9.0.0 precedent makes it a major release; it would add four runtime dependencies and a skeleton to every CLI project; and its strongest argument rests on movingcmemconto the template, which is an intention rather than a fact —cmemccarries no.copier-answers.ymltoday. The issue's own closing question, whether the entry point and output discipline ship as skeleton code or as skill prose, is the call that needs making.Nothing was declined, so Deliberate decisions — please do not re-raise these in
CLAUDE.mdis unchanged.Note on when the issues close
The closing keywords are in the commit subjects, not in this description — a
Fixes #NNhere would create no reference, because this pull request targetsdeveloprather than the default branch. The six issues therefore close when the commits reachmainat release time, which is later than a reviewer may expect.🤖 Generated with Claude Code
https://claude.ai/code/session_01Fzz1wJ17kdoc36w3mKufAy