Repository navigation
Conversation
|
I didn't have time yet to review the entire PR, but I did notice that many of the guides are duplicated from the generic plugin. Was that intentional? The point of the generic plugin is to avoid repeating the same advice in ever plugin. |
Yes it was intentional. 1. Several guides are modified for ruby. 2. I needed a different format for the line issues and unfortunately that is not easily plug-able without copying the guide and changing that code in the md. the line output is shown in this sample. it include the source ( |
Okay, that makes sense. Just to be clear: what follows is me trying to understand how to improve the framework itself to avoid having to repeat the guides in a plugin, not a criticism of this PR. So the part that I'd like to understand:
My design philosophy was to deliberately not include that, since it invites the agent to try and game the metric. In ideal case habit hooks is there to coach the agent on how to improve the code, and hide the actual signal as much as possible. (It tends to figure it out anyway if it really wants, but let's not make it easy) If the agent know the signal it starts to focus on making the signal go away instead of fixing the actual design issue.
I do see that casing is different in some files. Were there any other modifications necessary? (I could imagine a feature where the plugin specifies casing rules and the generic templates contain a formatting function call to ensure correct casing for the language) I'll now review the PR in more depth. |
|
@devill fyi - i am looking at the CI failures. i will have some updates soon. |
Use a writable Ruby toolchain on both CI platforms so the pinned RuboCop installation can complete, and keep the new plugin at the core release version.
Keep the Ruby sensor below the repository's file-size gate by separating process execution from report parsing and finding shaping.
Leave autofix metadata to RuboCop, and protect scoped filenames from option and glob interpretation before invoking the tool.
Keep vendored and temporary Rakefiles, Gemfiles, gemspecs, and rake files out of the Ruby scope without excluding other languages.
Keep a language-specific fallback from coaching findings produced by another language plugin; use the generic or core guide instead.
|
The core I’m still not really sure about this solution, though. I’m leaning on accepting it for now, but my main concern is that smells that are generic across languages should keep the generic prompt templates. This way, any improvement in one language can improve the experience for everyone. Copying and modifying the templates opts Ruby out of that. |
|
fyi - additional notes regarding the changes in the ruby guides:
|
|
@devill thanks! i've incorporated your changes and addressed the sensor failure case. This PR is ready for review again. |
Keep project-local tool directories in plugin configuration so only the tools that need them search those directories.
Keep path-list separators out of detector-owned search paths and document the plugin-scoped lookup precisely.
Keep bare argv programs on the same detector-owned search path used by setup and placeholder expansion.
|
Alright! I will look at the diffs between the duplicated prompts and the original. I'm still more in favour of modifying the generic prompts then duplicating the knowledge between languages. I'm open to having a ruby-prompts plugin though that has further customisations. |
|
The current change adds |
PR habit-hooks#150 forks nine prompts that already exist. Seven say the same thing once Ruby nouns are substituted, which the generic guides now carry as "function/method", so they are dropped and the generic guide renders. Two remain. parse-error states RuboCop facts no shared guide can hold: ruby -c, TargetRubyVersion, cops rather than rules. oversized-block is a smell only this plugin reports, so nothing else coaches it. The forked line-level include goes with them. It existed to print the cop's message, which is the metric the guides deliberately do not hand the agent, so oversized-block uses the shared listing.
|
Looking at the prompt changes there are many different categories of changes. For some I'd like to move them into generic, for some I very much disagree, and for some we should probably add features that let plugins customise vocabulary. In the interest of moving forward I suggest cutting the prompts from this PR with some notable exceptions (see other comments) and I will open separate issues with draft PRs for the other changes. Hope that is an acceptable trade-off for you. Since Habit hooks allows for project level overrides of guides you can continue using your variants by moving them to be project level overrides. I've put the cut on |
There was a problem hiding this comment.
I'd like to keep this one. While reading it I realised the generic guide you were overriding was written entirely for ESLint, so it shouldn't have been sitting in generic in the first place. I've moved it over to the typescript plugin and opened #153 to write a proper generic one.
Fun fact: the original was added by an agent unprompted and I didn't notice. Its wording was terrible, so the generic one will probably be based on yours instead. Gotta love agents... they love going behind your back when you rush :D
There was a problem hiding this comment.
I'd like to keep this one as well. For now only this plugin reports oversized-block, but since the feature itself exists in other languages it will probably be moved into generic with a slightly changed wording.
There was a problem hiding this comment.
This is the one I'd most like to drop, together with the uncoached guide that uses it.
It prints RuboCop's message, and the message carries the count — something like Cyclomatic complexity for foo is too high. [8/7]. I'd rather not hand the agent that number, because it invites gaming the metric. Sometimes I include the limit, but never the current value. Goodhart's law is real with AI agents. Especially the smarter ones. Once you give them the metric, they will aim for the easiest way to reach the target. (See also recent instances where frontier models hacked into places. In every single report the common thread was over optimising on getting to a metric instead of solving the problem.)
The cop name is already printed in the banner above each block, so you don't lose anything by falling back to the shared listing.
There was a problem hiding this comment.
in the interest of moving forward i am agreeable. i can experiment using project level coaching to understand how the agent behaves. if i find a significant difference, it can go into a future PR.
|
Once you rebase onto orign/main and cherry pick the temporary removal of the prompts from this PR I'll merge it. The prompts won't go lost, I'm opening the issues and draft PRs right now. |
|
fyi - i like most of the changes you recommended. i want to trial them as i incorporate them. so there will be a little delay to get everything updated. @devill question: have you considered getting habit hooks to also provide a human only output? the human view would provide the details of the failing sensor, scores, etc. this would allow the human to decide which sensor rules to enable/disable, and priorities about what to fix. it would also help the human in the loop to review the changes made by the ai. i have another unpublished project (thinking about publishing it this fall), which is a sidecar to run things like habit-hooks. it provides a UI to let the human see a human summary and details of the failing checks/sensors/evals, and it provides an API the AI can curl or hook into to get the ai written version of the checks/sensors. |
|
I didn't really consider that. My approach was to make sure I have to review as little as possible. But I'm open to adding something like that if you have a workflow that really needs it. |
|
@devill PR ready for review. I've incorporated your changes. I did a rewrite of the ruby plugin README.md to be a guide for ruby plugin setup and configuration, plus customization. The customization was more involved than it should be. Customization in some cases required copying py and toml files, rather than just making a small config or override change. Maybe the core of habit-hooks could make customization easier in the future. |
PR #150 forks nine prompts that already exist. Seven say the same thing once Ruby nouns are substituted, which the generic guides now carry as "function/method", so they are dropped and the generic guide renders. Two remain. parse-error states RuboCop facts no shared guide can hold: ruby -c, TargetRubyVersion, cops rather than rules. oversized-block is a smell only this plugin reports, so nothing else coaches it. The forked line-level include goes with them. It existed to print the cop's message, which is the metric the guides deliberately do not hand the agent, so oversized-block uses the shared listing.
|
Merged — thank you for putting in the effort. I rebased onto main so the history stays linear; your authorship is preserved. Four small fixes I added after merging:
|
Add a ruby plugin wrapping rubocop
Adds
habit-hooks-rubyplugin wrappingrubocop. The sensor runs the project's own rubocop and translates what comes back into the smell vocabulary, so a Rails app gets coaching instead of a cop list.What it does
A finding looks like this (one method tripping all three complexity cops, folded into a single coached block):
The decisions worth reviewing
The project's
.rubocop.ymldecides everything. The sensor passes no--onlyand no--config. RuboCop finds the project's config by its own upward walk, so a habit-hooks run is the run the project gets from rubocop directly. There is no shipped fallback config.--force-exclusionis the one flag added, because rubocop otherwise lints files named on the command line even whenAllCops: Excludecovers them.Ten cops map to canonical smells.
Metrics/ParameterListstotoo-many-parameters,MethodLengthtooversized-function, the three complexity cops tohigh-complexity, and so on.Metrics/ClassLengthandMetrics/ModuleLengthstay unmapped on purpose: they measure a class, and the vocabulary has no class-scoped smell for them to back.Unmapped cops are forwarded under their own names, the same exception eslint has. A cop that fired is one the project's own config turned on, so it is the project's vocabulary. It renders through
uncoached.mdand follows the rootuncoachedpolicy. The plugin ships its ownuncoached.mdthat keeps rubocop's message visible and points[Correctable]findings atrubocop -afirst.The binstub problem.
rubocopis a RubyGems binstub with a#!/usr/bin/env rubyshebang, only as good as therubythat answers first. Pointed at the wrong interpreter it dies infind_spec_for_exewith exit 1, the code reserved for "offences found". Judged on the exit code alone that is a false clean, the class #88 exists for. So the report is the evidence:--format jsonprints its envelope on every completed run, and no envelope means no run.project_pathsalso looks along the project'sbin/first, sobundle binstubs rubocopputs the bundled rubocop (the one that can loadrubocop-railsand friends) in front of whatever the machine has.Issue listings carry the cop and its message. The plugin ships
guides/includes/rubocop_line_level_issues.md, which rendersfile:line rubocop:<cop>with rubocop's message indented underneath. Acorrectableoffence adds rubocop's own[Correctable]tag to the source. RuboCop's JSON has no safety flag per offence, so the tag means an autocorrect exists, nothing more.Testing
plugins/ruby/docs/ruby-plugin.spec.md: the acceptance spec, same shape as the other plugins.test_the_rubocop_pipeline_maps_cops_to_smells.pysays what rubocop's output becomes with no subprocess;test_the_rubocop_sensor_runs_the_real_tool.pyproves the real tool still produces output of that shape. A rubocop upgrade that renamescop_namefails the second alone.Manually verified end-to-end against a real Rails app: the plugin runs the app's bundled rubocop, honours its inherited configs, and coached fabricated smells through both the mapped and the uncoached paths.
Notes for the reviewer
.github/workflows/ci.yml), andrelease.ymlmaps the new package to its own PyPI environment, per "Each released package needs its own publish environment".>=1.4.dev0,<2) and the floor gate passes.resolve_guide'suncoached.mdfallback uses rawconfig.pluginsorder rather than the language-ordered list it computed a line earlier. With two language plugins, an uncoached finding can render the other language's fallback guide. This plugin's ownuncoached.mdis what made that visible.