Add verify-procedure skill#18
Open
kquinn1204 wants to merge 15 commits intoredhat-documentation:mainfrom
Open
Conversation
Author
kquinn1204
force-pushed
the
add-verify-proc-skill-clean
branch
2 times, most recently
from
c3029f2 to
9c38d24
Compare
Collaborator
|
@kquinn1204 this is cool, have you been using the If it's not a one-size-fits-all thing then I'm wondering if it belongs in the docs-tools plugin, or if it's something specific for your project/team/work. If so you might want to put it in a new plugin. |
Author
|
@mcljot All good questions I have been using pretty consistently and feedback all results etc. into improving the tool. I have not tested with anything other than OCP clusters but think Aidan wanted this repo not product specific so feed that into the skill too. Though I think we may need place for product specific skills. Would love feedback on using with RHEL etc. |
aireilly
reviewed
Mar 13, 2026
Member
aireilly
left a comment
aireilly
left a comment
There was a problem hiding this comment.
Some thoughts!
-
re: "Magic steps" detection. Scanning for oc login, ssh, sudo, etc. as a proxy for "prerequisites are documented" is noisy. Many procedures use these commands within the actual procedure steps, not just in prerequisites.
-
The SKILL.md says it supports RHEL, Ansible, and OpenShift, but the implementation is heavily OCP-biased (dry-run via oc apply etc)
aireilly
force-pushed
the
main
branch
9 times, most recently
from
bd122fe to
0663a41
Compare
kquinn1204
force-pushed
the
add-verify-proc-skill-clean
branch
from
9c38d24 to
ec4f461
Compare
Author
|
@aireilly Added some more example and refined a bit more |
Member
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new Changes
Sequence DiagramsequenceDiagram
participant User
participant Verifier as verify_proc.rb
participant Parser as AsciiDoc Parser
participant TempFS as Temp Working Dir
participant Cluster as K8s/OCP Cluster
participant Executor as Step Executor
User->>Verifier: invoke with .adoc (+--cleanup?)
Verifier->>Parser: load and extract numbered steps & source blocks
Parser-->>Verifier: steps, labels, block types
Verifier->>Verifier: check .Prerequisites presence
Verifier->>TempFS: create temp working dir
loop for each step
Verifier->>Verifier: resolve attributes (subs)
alt needs product-version
Verifier->>Cluster: run oc/kubectl version -o json
Cluster-->>Verifier: version info / connectivity error
end
Verifier->>Executor: validate/execute block (by type)
alt YAML/JSON with apiVersion
Executor->>Executor: syntax check
Executor->>Cluster: kubectl/oc dry-run apply -f <file>
Cluster-->>Executor: validation result
else Bash
Executor->>TempFS: run command in temp dir (timeout)
TempFS-->>Executor: stdout/stderr
else Python/Ruby
Executor->>Executor: compile-time syntax check
else Config/Text
Executor->>TempFS: record files as needed
end
Executor-->>Verifier: step result + resource creation hints
Verifier->>Verifier: record created resources
end
alt --cleanup requested
Verifier->>Cluster: delete tracked k8s resources (reverse order)
Verifier->>Executor: stop/disable services, remove packages
Verifier->>TempFS: remove working dir
else no cleanup
Verifier->>TempFS: retain working dir
end
Verifier-->>User: summary of steps, pass/fail, first-line errors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/docs-tools/skills/verify-procedure/scripts/verify_proc.rb`:
- Around line 357-358: The JSON branch currently calls validate_json(content,
label) but ignores save_as, and other branches use File.basename which flattens
paths; replace those ad-hoc writes with a single helper (e.g.,
write_extracted_file(path, content, label:)) that enforces no absolute paths,
ensures writes stay under `@workdir`, mkdir_p the destination dir, writes content,
logs the saved path, and returns the dest; update the JSON handling (the call
site that invokes validate_json) to call write_extracted_file with the provided
save_as (when present) after validation, and replace any direct
File.basename-based writes in the other branches with calls to this helper to
preserve nested relative paths and consistent persistence.
- Around line 575-577: When parsing install commands in the block that matches
/\\b(?:dnf|yum)\\s+install\\s+(?:-y\\s+)?(.+)/, include the package manager (dnf
or yum) alongside the package list in `@created_resources` (e.g. { manager:
'dnf'|'yum', packages: [...] }) instead of storing only packages; then update
the cleanup code that currently always executes `dnf remove` to read the stored
manager for each created resource and run the corresponding remove command (`dnf
remove` or `yum remove`) for that resource's packages (preserve existing flag
filtering logic that rejects args starting with '-'). Ensure the key names match
`@created_resources` usage so removal picks the right manager per entry.
- Around line 570-571: The regex that parses systemctl invocations incorrectly
captures "--now" as the service; update the match on the string in the command
variable so it recognizes the "enable --now" variant before the plain "enable"
(e.g., match enable(?:\s+--now)? or reorder alternation) and ensure the captured
service is from the correct group (store the actual unit name in
`@created_resources` with action normalized to "enable" or "enable --now"/"start"
as appropriate). Also change the cleanup logic that iterates `@created_resources`
(the block that prints "[CLEANED]") to check the actual exit status of the
systemctl revert command and only mark resources cleaned on success, otherwise
report/propagate the failure instead of always printing "[CLEANED]".
- Around line 259-260: The third-level labels use ('i'..'xxvi').to_a which
relies on String#succ and produces lexicographic successors instead of Roman
numerals; replace that with a proper Roman-numeral lookup and use counters[2] to
index it (e.g., a ROMAN_THIRD_LEVEL array of "i","ii",...,"xxvi" or a small
to_roman method) and then push ROMAN_THIRD_LEVEL[counters[2]-1] into parts where
depth >= 3 and counters[2] > 0 (referencing the variables depth, counters, and
parts in the same conditional).
- Around line 517-524: The current Timeout.timeout wrapping of Open3.capture3
leaves the spawned child process running; replace this with an explicit
popen3-based runner (e.g., implement a run_command_with_timeout used instead of
Open3.capture3) that captures stdout/stderr via threads, closes stdin, uses
wait_thr to block for completion under Timeout.timeout, and on Timeout::Error
sends Process.kill('TERM', wait_thr.pid) then Process.kill('KILL', wait_thr.pid)
(rescuing exceptions), calls Process.wait(wait_thr.pid) (rescuing exceptions),
joins the reader threads and closes streams in ensure, and then return/raise so
your existing rescue block that logs the timeout and appends to `@results` remains
correct; reference Open3.capture3, Open3.popen3, wait_thr, Process.kill, and
Timeout.timeout when locating the change.
In `@plugins/docs-tools/skills/verify-procedure/SKILL.md`:
- Line 56: The markdown has unlabeled fenced code blocks triggering MD040;
update each unlabeled triple-backtick fence in this skill doc to include a
language specifier (e.g., change ``` to ```text or ```console) for the examples
referenced in the comment so the blocks at the three reported locations are
explicitly labeled and the document becomes lint-clean per MD040.
- Around line 27-31: Update the example invocations in SKILL.md to call the
verify_proc.rb script using a relative path from the skill directory instead of
the placeholder `<skill_dir>`; replace the two lines that show `ruby
<skill_dir>/scripts/verify_proc.rb <file.adoc>` and `ruby
<skill_dir>/scripts/verify_proc.rb --cleanup <file.adoc>` with invocations that
use the relative script path (e.g., start with `ruby scripts/verify_proc.rb`) so
the instructions match the repo's skill-invocation convention and will run
correctly in the skill environment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 97b23072-d131-480b-9e54-c7a525607d0f
📒 Files selected for processing (4)
.claude-plugin/marketplace.jsonplugins/docs-tools/.claude-plugin/plugin.jsonplugins/docs-tools/skills/verify-procedure/SKILL.mdplugins/docs-tools/skills/verify-procedure/scripts/verify_proc.rb
There was a problem hiding this comment.
♻️ Duplicate comments (1)
plugins/docs-tools/skills/verify-procedure/scripts/verify_proc.rb (1)
388-396:⚠️ Potential issue | 🟠 MajorAdd path traversal protection when writing extracted files.
The current check rejects absolute paths but doesn't prevent directory traversal via
../. A malicious AsciiDoc file could write files outside the working directory using a path like../../etc/malicious.yaml.🛡️ Proposed fix using path canonicalization
+ # Ensure a path stays within the working directory (prevents traversal attacks) + def safe_workdir_path(relative_path) + return nil if relative_path.start_with?('/') || relative_path.start_with?('~/') + + dest = File.expand_path(relative_path, `@workdir`) + workdir_root = File.expand_path(`@workdir`) + File::SEPARATOR + return nil unless dest.start_with?(workdir_root) + + FileUtils.mkdir_p(File.dirname(dest)) + dest + end + def validate_yaml(content, label, save_as = nil) begin # Lint the YAML for syntax errors YAML.safe_load(content) puts "[VALID] YAML syntax for Step #{label} is correct." `@results` << { step: label, status: :passed, output: "YAML syntax valid" } if save_as - if save_as.start_with?('/') || save_as.start_with?('~/') + dest = safe_workdir_path(save_as) + if dest.nil? puts "[INFO] Absolute path #{save_as} — YAML validated but not written to filesystem." else - dest = File.join(`@workdir`, save_as) - FileUtils.mkdir_p(File.dirname(dest)) File.write(dest, content) puts "[INFO] Saved YAML to #{dest}" end endApply the same pattern to
validate_json(line 436),validate_config(line 459), andvalidate_script(line 499).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/docs-tools/skills/verify-procedure/scripts/verify_proc.rb` around lines 388 - 396, The save logic currently only blocks absolute paths but allows directory traversal; update the save handling in the method (the block that checks save_as and writes dest) to canonicalize and validate the destination is inside the working directory: compute dest = File.expand_path(File.join(`@workdir`, save_as)) and base = File.expand_path(`@workdir`), then if dest.start_with?(base + File::SEPARATOR) (or equals base) proceed to mkdir_p and File.write, otherwise log an error/refuse to write; apply the same canonicalization+containment check to the corresponding save blocks in validate_json, validate_config, and validate_script methods to prevent ../ path traversal.
🧹 Nitpick comments (4)
plugins/docs-tools/skills/verify-procedure/scripts/verify_proc.rb (4)
488-488: Prefer array form forOpen3.capture3to avoid shell interpretation.Using string interpolation passes the command through a shell, which Brakeman flags as a potential injection risk. Although
f.pathfrom Tempfile is safe, the array form is more defensive.♻️ Proposed fix
- stdout, stderr, status = Open3.capture3("ruby -c #{f.path}") + stdout, stderr, status = Open3.capture3('ruby', '-c', f.path)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/docs-tools/skills/verify-procedure/scripts/verify_proc.rb` at line 488, Open3.capture3 is being called with a single interpolated string ("ruby -c #{f.path}") which invokes a shell; change it to the array form to avoid shell interpretation and injection risk by calling Open3.capture3 with separate arguments, e.g. pass "ruby", "-c", and f.path as distinct parameters so the call becomes Open3.capture3("ruby", "-c", f.path) and continue to use the same stdout, stderr, status variables.
633-647: Use array form for cleanup commands to prevent shell interpretation.The
res[:file]andres[:resource]values originate from AsciiDoc content. Using array form ensures filenames with spaces or special characters are handled correctly.♻️ Proposed fix
- stdout, stderr, status = Open3.capture3("#{tool} delete -f #{res[:file]} --ignore-not-found") + _, stderr, status = Open3.capture3(tool, 'delete', '-f', res[:file], '--ignore-not-found')- stdout, stderr, status = Open3.capture3("#{tool} delete #{res[:resource]} --ignore-not-found") + _, stderr, status = Open3.capture3(tool, 'delete', res[:resource], '--ignore-not-found')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/docs-tools/skills/verify-procedure/scripts/verify_proc.rb` around lines 633 - 647, The cleanup commands use Open3.capture3 with a single string which invokes the shell and can mis-handle filenames from AsciiDoc (res[:file], res[:resource]); change both calls to the array form of Open3.capture3 to pass the command and args as separate elements (use tool = `@cli_tool` || 'oc' and then call Open3.capture3(tool, 'delete', '-f', res[:file], '--ignore-not-found') and Open3.capture3(tool, 'delete', res[:resource], '--ignore-not-found') respectively) so spaces and special characters are passed safely without shell interpretation.
140-143: Remove or use thecurrent_step_depthvariable.
current_step_depthis assigned at lines 142 and 180 but never read. RuboCop flags this as a useless assignment.♻️ Proposed fix
# Hierarchical step tracking: [major, sub, subsub] step_counters = [0, 0, 0] current_step_label = nil - current_step_depth = 0And at line 180:
current_step_label = format_step_label(step_counters, depth) - current_step_depth = depth current_step_text = step_text🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/docs-tools/skills/verify-procedure/scripts/verify_proc.rb` around lines 140 - 143, Remove the unused local variable current_step_depth (its initial assignment and any later reassignments) since it's never read; locate the declarations/assignments to current_step_depth near the step processing logic (alongside step_counters and current_step_label) and delete them, or if there was an intended use, implement the missing read logic where step depth should influence processing instead of keeping the unused variable.
408-408: Use underscore prefix for unused capture variable.The
stdoutvariable from thisOpen3.capture3call is never used. Use_or_stdoutto signal intent.♻️ Proposed fix
- stdout, stderr, status = Open3.capture3("#{`@cli_tool`} apply -f #{f.path} --dry-run=client") + _, stderr, status = Open3.capture3("#{`@cli_tool`} apply -f #{f.path} --dry-run=client")Also applies to line 663 in
run_cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/docs-tools/skills/verify-procedure/scripts/verify_proc.rb` at line 408, Two Open3.capture3 calls capture stdout into an unused variable named stdout; update those captures to use an underscore-prefixed name (e.g., _stdout or _) to indicate the value is intentionally unused. Locate the capture at the call using Open3.capture3("#{`@cli_tool`} apply -f #{f.path} --dry-run=client") and the similar capture inside run_cleanup, and rename stdout to _stdout (or _) and adjust any parallel assignment (stderr, status) accordingly so the rest of the method still uses stderr and status as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@plugins/docs-tools/skills/verify-procedure/scripts/verify_proc.rb`:
- Around line 388-396: The save logic currently only blocks absolute paths but
allows directory traversal; update the save handling in the method (the block
that checks save_as and writes dest) to canonicalize and validate the
destination is inside the working directory: compute dest =
File.expand_path(File.join(`@workdir`, save_as)) and base =
File.expand_path(`@workdir`), then if dest.start_with?(base + File::SEPARATOR) (or
equals base) proceed to mkdir_p and File.write, otherwise log an error/refuse to
write; apply the same canonicalization+containment check to the corresponding
save blocks in validate_json, validate_config, and validate_script methods to
prevent ../ path traversal.
---
Nitpick comments:
In `@plugins/docs-tools/skills/verify-procedure/scripts/verify_proc.rb`:
- Line 488: Open3.capture3 is being called with a single interpolated string
("ruby -c #{f.path}") which invokes a shell; change it to the array form to
avoid shell interpretation and injection risk by calling Open3.capture3 with
separate arguments, e.g. pass "ruby", "-c", and f.path as distinct parameters so
the call becomes Open3.capture3("ruby", "-c", f.path) and continue to use the
same stdout, stderr, status variables.
- Around line 633-647: The cleanup commands use Open3.capture3 with a single
string which invokes the shell and can mis-handle filenames from AsciiDoc
(res[:file], res[:resource]); change both calls to the array form of
Open3.capture3 to pass the command and args as separate elements (use tool =
`@cli_tool` || 'oc' and then call Open3.capture3(tool, 'delete', '-f', res[:file],
'--ignore-not-found') and Open3.capture3(tool, 'delete', res[:resource],
'--ignore-not-found') respectively) so spaces and special characters are passed
safely without shell interpretation.
- Around line 140-143: Remove the unused local variable current_step_depth (its
initial assignment and any later reassignments) since it's never read; locate
the declarations/assignments to current_step_depth near the step processing
logic (alongside step_counters and current_step_label) and delete them, or if
there was an intended use, implement the missing read logic where step depth
should influence processing instead of keeping the unused variable.
- Line 408: Two Open3.capture3 calls capture stdout into an unused variable
named stdout; update those captures to use an underscore-prefixed name (e.g.,
_stdout or _) to indicate the value is intentionally unused. Locate the capture
at the call using Open3.capture3("#{`@cli_tool`} apply -f #{f.path}
--dry-run=client") and the similar capture inside run_cleanup, and rename stdout
to _stdout (or _) and adjust any parallel assignment (stderr, status)
accordingly so the rest of the method still uses stderr and status as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72dbb89c-8794-4550-9782-e700e6f20ef2
📒 Files selected for processing (2)
plugins/docs-tools/skills/verify-procedure/SKILL.mdplugins/docs-tools/skills/verify-procedure/scripts/verify_proc.rb
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
plugins/docs-tools/skills/verify-procedure/scripts/verify_proc.rb (4)
483-483: Prefix unusedstdoutwith underscore.RuboCop flags this as an unused variable. Use
_stdoutto indicate intentional non-use.- stdout, stderr, status = Open3.capture3('python3', '-c', "compile(#{content.inspect}, '<step #{label}>', 'exec')") + _stdout, stderr, status = Open3.capture3('python3', '-c', "compile(#{content.inspect}, '<step #{label}>', 'exec')")Note: The Brakeman command injection warning here is a false positive—array form prevents shell interpretation, and
compile()only parses without executing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/docs-tools/skills/verify-procedure/scripts/verify_proc.rb` at line 483, RuboCop warns that the variable stdout returned by Open3.capture3 in the call inside verify_proc.rb is unused; rename it to _stdout to indicate intentional non-use (i.e., change the assignment from stdout, stderr, status = Open3.capture3(...) to _stdout, stderr, status = Open3.capture3(...)) so the linter stops flagging it while preserving the existing compile call and behavior.
543-600: Note: Shell command execution is by design but requires trusted input.The tool intentionally uses shell interpretation (string form in popen3) to support documentation commands with pipes, redirects, and other shell features. This is necessary for executing real procedure steps but means the tool should only process trusted AsciiDoc files.
Consider adding a warning in the CLI usage or SKILL.md documentation about only running this on trusted content.
Would you like me to draft documentation text warning about running this only on trusted AsciiDoc files?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/docs-tools/skills/verify-procedure/scripts/verify_proc.rb` around lines 543 - 600, Add an explicit security warning about executing untrusted AsciiDoc files: update the skill documentation (SKILL.md) and the CLI usage/help text to state that execute_bash (and overall command execution that uses run_command_with_timeout) performs shell interpretation and must only be used with trusted AsciiDoc content; also add a short in-code comment or log message near the top of execute_bash noting the same trusted-input requirement so users running the script see the warning at runtime.
166-176: Minor: Redundant step counter reset logic.Lines 167-168 reset counters for deeper levels using the loop, but lines 171-176 explicitly reset the same counters again. The explicit resets are redundant but arguably clearer for readers.
Simplified version (optional)
# Update counters: increment at this depth, reset deeper levels step_counters[depth - 1] += 1 (depth...3).each { |d| step_counters[d] = 0 } - - # Reset sub-counters when a new major step starts - if depth == 1 - step_counters[1] = 0 - step_counters[2] = 0 - elsif depth == 2 - step_counters[2] = 0 - end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/docs-tools/skills/verify-procedure/scripts/verify_proc.rb` around lines 166 - 176, The code redundantly resets deeper step counters twice: once via the loop (step_counters[depth - 1] += 1; (depth...3).each { |d| step_counters[d] = 0 }) and again in the explicit if/elsif block; remove the explicit reset block (the if depth == 1 / elsif depth == 2 section) and keep the loop-based reset to avoid duplication while preserving the same behavior for step_counters and depth.
115-133: Consider replacing barerescuewith specific exception types.The bare
rescueat line 125 catches all StandardError subclasses, which could mask unexpected errors during JSON parsing or version extraction.Suggested fix
begin data = JSON.parse(stdout) # oc version returns serverVersion or openshiftVersion `@cluster_version` = data.dig('openshiftVersion')&.match(/^(\d+\.\d+)/)&.[](1) `@cluster_version` ||= data.dig('serverVersion', 'minor')&.then { |m| "1.#{m}" } - rescue + rescue JSON::ParserError, TypeError, NoMethodError `@cluster_version` = nil endNote: The Brakeman command injection warning for line 118 is a false positive since
@cli_toolis constrained to 'oc' or 'kubectl' bydetect_cli_tool.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/docs-tools/skills/verify-procedure/scripts/verify_proc.rb` around lines 115 - 133, In detect_cluster_version replace the bare rescue with targeted exception handling: rescue JSON::ParserError, NoMethodError, TypeError => e (or whichever subset fits your runtime) so only JSON parsing and string/struct access errors are caught; inside that rescue set `@cluster_version` = nil (and optionally log or attach e.message) instead of swallowing all exceptions. This change limits the catch to errors from JSON.parse and subsequent dig/match operations while leaving other unexpected errors to propagate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/docs-tools/skills/verify-procedure/scripts/verify_proc.rb`:
- Around line 251-262: The format_step_label method can index past the bounds of
the alphabet and roman arrays when counters[1] or counters[2] exceed the arrays'
lengths, producing nil parts; update format_step_label to guard each array
access: for the depth>=2 alphabet part (currently using
('a'..'z').to_a[counters[1] - 1]) and the depth>=3 roman part (roman[counters[2]
- 1]) check that (counters[1]-1) and (counters[2]-1) are within the arrays'
index range before using them, and if out of range append a sensible fallback
(e.g., counters[1].to_s and counters[2].to_s or a generated multi-letter
sequence) so no nil is pushed into parts and labels remain meaningful; keep the
checks inside format_step_label where counters and depth are used.
---
Nitpick comments:
In `@plugins/docs-tools/skills/verify-procedure/scripts/verify_proc.rb`:
- Line 483: RuboCop warns that the variable stdout returned by Open3.capture3 in
the call inside verify_proc.rb is unused; rename it to _stdout to indicate
intentional non-use (i.e., change the assignment from stdout, stderr, status =
Open3.capture3(...) to _stdout, stderr, status = Open3.capture3(...)) so the
linter stops flagging it while preserving the existing compile call and
behavior.
- Around line 543-600: Add an explicit security warning about executing
untrusted AsciiDoc files: update the skill documentation (SKILL.md) and the CLI
usage/help text to state that execute_bash (and overall command execution that
uses run_command_with_timeout) performs shell interpretation and must only be
used with trusted AsciiDoc content; also add a short in-code comment or log
message near the top of execute_bash noting the same trusted-input requirement
so users running the script see the warning at runtime.
- Around line 166-176: The code redundantly resets deeper step counters twice:
once via the loop (step_counters[depth - 1] += 1; (depth...3).each { |d|
step_counters[d] = 0 }) and again in the explicit if/elsif block; remove the
explicit reset block (the if depth == 1 / elsif depth == 2 section) and keep the
loop-based reset to avoid duplication while preserving the same behavior for
step_counters and depth.
- Around line 115-133: In detect_cluster_version replace the bare rescue with
targeted exception handling: rescue JSON::ParserError, NoMethodError, TypeError
=> e (or whichever subset fits your runtime) so only JSON parsing and
string/struct access errors are caught; inside that rescue set `@cluster_version`
= nil (and optionally log or attach e.message) instead of swallowing all
exceptions. This change limits the catch to errors from JSON.parse and
subsequent dig/match operations while leaving other unexpected errors to
propagate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7414cb7-729f-40b2-aa9c-c28afcc9a9d3
📒 Files selected for processing (1)
plugins/docs-tools/skills/verify-procedure/scripts/verify_proc.rb
kquinn1204
force-pushed
the
add-verify-proc-skill-clean
branch
from
d3d3ce6 to
d41c6a9
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/docs-tools/skills/verify-procedure/scripts/verify_proc.rb`:
- Around line 604-618: The track_resource method only records the first regex
match for command and stdout, so multiple created resources or files are missed;
update track_resource to iterate all matches: for the command pattern
(/(?:\b(?:oc|kubectl)\s+(?:create|apply)\s+-f\s+(\S+))/) use scan or split the
command into tokens/lines and append an entry to `@created_resources` for each
matched file (preserving tool via capture or re-parsing), and for the stdout
pattern (%r{^(\S+/\S+)\s+created}) iterate each matching line (e.g., each_line +
scan) and push a { resource: match } for every occurrence; apply the same
multi-match approach to the similar logic referenced around lines 625-629 so all
created packages/resources are tracked rather than only the first match.
- Around line 285-303: The link_yaml_to_files extractor (see
SAVE_FILE_EXTENSIONS and method link_yaml_to_files) currently skips
home-relative paths and un-extended quoted filenames; update link_yaml_to_files
to 1) normalize home-relative paths by mapping leading "~/..." into the
sandboxed workdir (use the existing safe_workdir_path helper or equivalent to
translate "~/" into the `@workdir` prefix) so backtick-quoted `~/playbook.yml` is
materialized, and 2) add an additional matching branch to capture quoted
relative filenames without extensions (e.g., `/\`([^`\/]+)\`/` for simple quoted
names like `default`) and synthesize a sensible save_as filename (appending a
default extension or placing under `@workdir`) so subsequent commands reference
the materialized file; ensure these new branches set block[:save_as]
consistently with the other cases and do not override existing absolute-path
handling.
- Around line 186-190: The current check sets has_subs =
source_attrs.include?('subs=') which treats any subs override (e.g., subs=quotes
or subs=none) as enabling attribute substitution; instead parse the subs value
from source_attrs (the part after 'subs='), split it into tokens on commas,
normalize each token (strip whitespace) and only set has_subs = true when one of
the tokens explicitly enables attributes (e.g., token == 'attributes' or token
== '+attributes' or otherwise contains 'attributes' in the enabling form); then
only call resolve_attributes when that parsed has_subs is true (update the
regex-handling block that sets source_type/source_attrs and the subsequent
resolve_attributes call to use this more precise check).
- Around line 620-623: The code currently pushes a hash with service and raw
action into `@created_resources` but cleanup always runs both stop and disable;
change the capture to store a normalized action type (e.g. "start", "enable", or
"enable--now") when matching the regex in the systemctl detection block (the
code that currently uses `@created_resources` << { service: $2, action: $1 }) and
update the cleanup logic to reverse only that action: map "start" => run stop,
"enable" => run disable, and "enable--now" (or equivalent flagged value) => run
both stop and disable; apply the same normalization and cleanup mapping for the
duplicate block referenced (lines ~656-665) so each recorded resource is
reverted exactly as performed.
- Around line 287-289: link_yaml_to_files currently only treats block[:type] ==
%w[yaml json config] as file content and always executes script blocks (the bash
branch), so saved script snippets get executed instead of being written; modify
link_yaml_to_files to include script types (e.g.
'bash','sh','python','ruby','script') in the save-target detection (extend the
%w[...] check) and, inside the bash/script execution branch, short-circuit
execution when the block carries a save target (check for block[:save_to] ||
block[:filename] || block[:file] || block.dig(:meta,:save) or similar) and write
the content to the file instead of running it; apply the same change to the
duplicate script-handling logic later in the file that mirrors the bash branch
so saved scripts are never executed prematurely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ad07d59b-5718-4511-a549-b2e41859a69a
📒 Files selected for processing (2)
plugins/docs-tools/skills/verify-procedure/SKILL.mdplugins/docs-tools/skills/verify-procedure/scripts/verify_proc.rb
kquinn1204
force-pushed
the
add-verify-proc-skill-clean
branch
3 times, most recently
from
e6d8d29 to
264ffbd
Compare
Adds a new docs-tools skill that executes AsciiDoc procedures against a live system to prove they work end-to-end. Requires an active connection (oc login, SSH, etc.) — for offline review, use the technical-reviewer agent instead. The Ruby script (verify_proc.rb) parses the .adoc file, extracts numbered steps and their source blocks, then runs each one sequentially. It validates YAML syntax and dry-runs Kubernetes resources, executes bash commands, skips example output and placeholder blocks, and reports a pass/fail summary. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…add ifdef warnings Address five validation issues: - Replace noisy magic-steps command scanning with .Prerequisites section check - Make dry-run validation CLI-agnostic (works with both oc and kubectl) - Add ifdef/ifndef conditional warnings instead of silently mis-numbering steps - Broaden file-save detection to match real-world phrasing from openshift-docs - Replace hardcoded OCP attributes with document-extracted attribute resolution Bump docs-tools version to 0.0.11. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extend the skill beyond OCP to handle RHEL documentation patterns: - Parse ini, toml, text, python, ruby source block types - Strip RHEL root prompts (#, [root@host]#, ~]#) in addition to $ - Validate Python and Ruby script blocks for syntax errors - Handle absolute paths (/etc/foo.conf) — validate but never write - Track systemctl and dnf/yum changes for cleanup - Add RHEL examples to SKILL.md from NotebookLM analysis Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When no cluster is connected, the dry-run via oc/kubectl fails with connection errors. Previously this replaced the YAML syntax pass with a failure (via @results.pop), making all valid YAML blocks appear broken. Now: - Connection errors (no such host, connection refused) skip the dry-run gracefully while preserving the syntax pass - Genuine resource errors (invalid field, unknown kind) are recorded as a separate result instead of replacing the syntax check Before: 0 passed / 7 failed (no cluster) After: 3 passed / 4 failed (YAML syntax passes preserved) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix Roman numeral labels: replace ('i'..'xxvi').to_a (produces i,j,k)
with explicit Roman numeral array
- Fix extracted file writes: pass save_as to validate_json, preserve
nested paths instead of flattening with File.basename, mkdir_p parents
- Fix command timeout: add run_command_with_timeout using popen3 with
explicit TERM/KILL to terminate child processes on timeout
- Fix package manager tracking: capture dnf vs yum during install and
use the same manager during cleanup
- Fix systemctl regex: reorder alternation so 'enable --now' matches
before 'enable', check exit codes during cleanup
- Add language identifiers to unlabeled fenced code blocks in SKILL.md
- Use relative script path in SKILL.md per repo conventions
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add safe_workdir_path to prevent directory traversal via ../ in extracted file paths; use it for all file writes (YAML, JSON, config, script) - Use array form for Open3.capture3 calls to avoid shell interpretation: cli_tool dry-run, ruby syntax check, cleanup delete commands - Remove unused current_step_depth variable - Use _ prefix for unused stdout captures Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The previous check `source_attrs.include?('subs=')` matched any subs
override (e.g. subs=none, subs=quotes), which could corrupt literal
{...} content. Now only triggers attribute resolution when the subs
value explicitly includes 'attributes'.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Map ~/... paths into the sandboxed workdir instead of dropping them - Add rule for backtick-quoted relative filenames without extensions (e.g., `default`, `my-config`) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fall back to "sub<N>" when depth-2 or depth-3 counters exceed the 26-element alpha/roman arrays, preventing nil labels. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Steps like "Create verify.sh with the following content" now save the bash/python/ruby block to disk instead of executing it. This ensures later steps that reference the file (e.g., python foo.py) can find it. - Extend link_yaml_to_files to detect save targets for all block types - Add SAVE_INSTRUCTION_PATTERN to identify "create/save/write file" - Short-circuit execution when a script block should be saved - Add .py and .rb to SAVE_FILE_EXTENSIONS Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace single =~ regex matches with scan to iterate all matches in commands and stdout. Fixes cleanup leaving behind resources when a block applies multiple manifests, installs multiple packages, or prints several "created" lines. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- start → stop (don't change enablement) - enable → disable (don't stop) - enable --now → stop and disable Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
YAML.safe_load fails on multi-document files (multiple --- separated docs) common in K8s manifests. safe_load_stream handles these correctly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace the 782-line Ruby parser/executor with a two-layer architecture: - Claude handles AsciiDoc parsing, intent classification, prompt stripping, step numbering, and judgment calls (include:: resolution, ifdef handling, nested blocks, callout stripping) - A thin 397-line bash script (verify_proc.sh) handles only execution: run commands, validate YAML/JSON, save files, track resources, cleanup Script improvements over the Ruby version: - Session-isolated temp files (concurrent runs don't collide) - Safe counter reads (grep/cut instead of source) - printf instead of echo (no spurious trailing newlines) - Python for YAML/JSON validation (more portable) SKILL.md improvements: - Pushier description for better skill triggering - Heredoc examples instead of fragile echo piping - Guidance on assemblies (multi-file procedures) - Guidance on partial cluster state and missing prerequisites - Focused on OpenShift/K8s (dropped RHEL/Ansible/Python/Ruby scope) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Profiles are Markdown files stored in this repo that configure verify-procedure runs against procedures in external repositories. They provide attribute values, placeholder substitutions, prerequisites, scope controls, assertions, and cleanup overrides for repeatable verification without interactive guidance. - Add profiles section and discovery logic to SKILL.md - Add substitution, scope, and assertion processing steps - Support VERIFY_TIMEOUT env var in verify_proc.sh - Add profile-format.md reference for profile authors - Add design spec: specs/2026-03-25-verify-procedure-profiles.md - Create profiles/ directory for future profile files Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
kquinn1204
force-pushed
the
add-verify-proc-skill-clean
branch
from
264ffbd to
3b784bc
Compare
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
3 participants
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.
Summary
verify-procedureskill to thedocs-toolspluginSKILL.mddefining the skill andscripts/verify_proc.rbfor procedure verification on a live clusterTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit