Skip to content

001 collection consistency audit - #63

Merged
dmartinol merged 12 commits into
RHEcosystemAppEng:mainfrom
RoddieKieley:001-collection-consistency-audit
May 7, 2026
Merged

001 collection consistency audit#63
dmartinol merged 12 commits into
RHEcosystemAppEng:mainfrom
RoddieKieley:001-collection-consistency-audit

Conversation

@RoddieKieley

Copy link
Copy Markdown
Contributor

Summary

This was created for APPENG-4927, checking the consistency of the agentic-collections pack. It is very much DRAFT and needs review to see what is valuable information versus what might be valuable to actually commit to the project repository itself.

Pack(s) affected

  • rh-sre
  • rh-developer
  • ocp-admin
  • rh-virt
  • rh-ai-engineer
  • Other / repo-wide

Change type

  • New skill
  • New agent
  • New pack
  • Update existing skill / agent
  • MCP server config (mcps.json)
  • Docs / README
  • CI / tooling

CLAUDE.md compliance

  • Agents orchestrate skills; no direct MCP/tool calls in agents
  • Skills are single-purpose task executors
  • Skills encapsulate all tool access (MCP tools invoked only inside skills)
  • Document consultation: file is read with the Read tool, then declared to the user
  • No credentials hardcoded; env vars used via ${VAR} references
  • Human-in-the-loop confirmation added for any destructive or critical operations

Validation

  • make validate passes locally
  • New/changed skills have valid YAML frontmatter (name, description)
  • New/changed agents have valid YAML frontmatter (name, description)

Signed-off-by: Roddie Kieley <rkieley@redhat.com>
Signed-off-by: Roddie Kieley <rkieley@redhat.com>
Signed-off-by: Roddie Kieley <rkieley@redhat.com>
Signed-off-by: Roddie Kieley <rkieley@redhat.com>
…ration check

Signed-off-by: Roddie Kieley <rkieley@redhat.com>
Re-run the consistency audit workflow after rebasing the 001 branch onto current main and update generated JSON/Markdown findings.

Made-with: Cursor
@RoddieKieley
RoddieKieley requested a review from dmartinol April 21, 2026 13:07
@github-actions

Copy link
Copy Markdown

Gemini Code Review

1. Summary

This PR introduces a comprehensive "Collection Consistency Audit" framework to ensure metadata, versioning, style tokens, and documentation claims remain aligned across the repository. It adds Python-based audit scripts, updates CI workflows to enforce these checks, standardizes skill frontmatter (model and color fields) across several rh-sre skills, and introduces a canonical style token policy for the docs site.

2. Project Rules Compliance

  • Frontmatter fields: The PR correctly updates several skills in rh-sre to include the mandatory model and color fields, and fixes a malformed frontmatter block in remediation/SKILL.md by removing an errant --- that was prematurely closing the YAML block. It also correctly updates the validators to enforce these fields.
  • Build reminder: Since skills were modified (rh-sre/skills/*/SKILL.md), please ensure you run make validate locally to verify all changes pass the updated structure and design checks.

3. Code Quality Issues

The code quality is excellent. The audit framework is modular, well-structured, and relies on the standard library, avoiding unnecessary external dependencies.

One minor observation regarding repository hygiene:

  • Committing Generated Reports: The PR commits reports/ci-violations.json, reports/consistency-audit.json, and reports/consistency-audit.md. Generated audit reports are typically ephemeral and shouldn't be tracked in version control, as they will cause constant merge conflicts and dirty working trees whenever metadata changes.

4. Suggestions

  • Ignore Reports Directory: Consider removing the committed files in the reports/ directory from version control and adding the directory to .gitignore. The reports are generated dynamically in CI and locally, so tracking them in Git is unnecessary and prone to causing noise.
    # Add to .gitignore
    reports/
  • Graceful File Handling: In scripts/consistency_audit_lib/checks/style_checks.py and icon_mapping_checks.py, the scripts assume files like docs/styles.css and docs/icons.json always exist. While true for the current repo state, adding a quick .exists() check before reading them (similar to how you handled docs/data.json) would make the audit script more robust against future structural changes.

5. Verdict

APPROVE — The PR is exceptionally well-designed, implements a highly valuable consistency framework, and correctly aligns existing skills with the project's frontmatter policies. Consider the suggestion regarding the reports/ directory before merging.


Automated review by Gemini 3.1 Pro | Rules: CLAUDE.md, SKILL_DESIGN_PRINCIPLES.md

@dmartinol dmartinol left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your first contribution!

Let's discuss offline how we can integrate these changes. Main concerns:

  • Adoption of spec-kit which was not used before (at least document the install and howtos)
  • The audit reported some issues that need to be addressed, which was the main goal of the task
  • If we really want a continuous audit, shouldn't we integrate these new checks into the existing Make targets?

|---|---|---|---|---|---|
| `VER-001` | marketplace | blocking | computed | false | `marketplace/rh-agentic-collection.yml` |
| `VER-002` | pack | high | computed | true | `README.md`, `<pack>/README.md` |
| `VER-003` | pack | medium | schema | true | `<pack>/.claude-plugin/plugin.json` (if present) |

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these plugin.json were deleted from the repo since when we adopted lola

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on the other hand, we must have a collection.yaml file under the .catalog folder (but this is already checked by the collection_validate python script)

Comment thread reports/consistency-audit.json Outdated
],
"findings": [
{
"finding_id": "VER-001-rh-support-engineer",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rh-support-engineer has been removed from the repo, where does this come from?

Comment thread reports/consistency-audit.json Outdated
"rule_id": "VIS-003",
"severity": "high",
"artifact_path": "docs/data.json",
"message": "Documentation site data file is missing or has not yet been generated",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is in the .gitignore list because it's only generated in the GH pages

# Generated Documentation
docs/data.json

Comment thread reports/consistency-audit.md Outdated

## Matrix

| Pack | Registration | Version | Model | Claims | Style | Overall |

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not clear where the warns come from: we should add the explanation to the findings section.

Comment thread reports/consistency-audit.md Outdated

- [informational] `VER-001` Pack 'rh-support-engineer' is not listed in marketplace modules (marketplace/rh-agentic-collection.yml)
- [high] `VER-002` Pack README missing for 'rh-support-engineer' (rh-support-engineer/README.md)
- [high] `CLM-001` Root README total skill count is out of sync with repository reality (README.md)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably CLM-001 is the only real issue, the other will disappear as we integrate the previous comments

- NEVER execute playbooks using ansible-playbook CLI
- ALWAYS delegate execution to /playbook-executor skill
model: inherit
color: green

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not a read-only skill, so I think the color severity should be raised (maybe not red but at least yellow, WDYT?)

Comment thread scripts/validate_skill_design.py Outdated
result.errors.append(f"Frontmatter missing required field: {field_name}")

if "model" in frontmatter:
valid_models = {"inherit", "sonnet", "haiku"}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one thing I wanted to verify with this consistency audit: is it really worth to have model values different from inherit?

| `medium` | warn | warn (optionally fail on changed scope later) |
| `informational` | report | report |

## 5) Rh-Support-Engineer Decision Branch Model

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be completely removed IMO

| green | Success, deployment |
| yellow | Caution, validation |
| red | Critical, security, remediation |
| magenta | Creative, generation |

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we restore this line?

@RoddieKieley

Copy link
Copy Markdown
Contributor Author
* Adoption of `spec-kit` which was not used before (at least document the install and howtos)

The spec-kit is not itself included as it is in the .specify directory as well as skill entries that are in directories .claude/skills/speckit-*. That being said the result of using spec-kit which is the output found in the included specs directory. While that does create some noise for humans I have found it to be useful in providing context over time when you further use ai to do something and you are double digit specs into development within a project.

* The audit reported some issues that need to be addressed, which was the main goal of the task

Indeed there were some issues but unfortunately I took the title of APPENG-4927, "Consistency check of all collections (e.g. using same version, model, color, etc etc)" literally and took a pass to undo some of the changes.

That being said we should update the title to be action oriented, identify the changes required, and get them implemented.

* If we really want a continuous audit, shouldn't we integrate these new checks into the existing Make targets?

Those were some of the changes that were undone and not included. There were new make targets however I backed off to a check and providing an audit.

dmartinol and others added 5 commits May 4, 2026 11:56
Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
Signed-off-by: Roddie Kieley <rkieley@redhat.com>
Signed-off-by: Roddie Kieley <rkieley@redhat.com>
…012 completed post latest main rebase.

Signed-off-by: Roddie Kieley <rkieley@redhat.com>
…ass and update.

Signed-off-by: Roddie Kieley <rkieley@redhat.com>
@RoddieKieley
RoddieKieley marked this pull request as ready for review May 4, 2026 14:27
Comment thread .github/workflows/compliance-check.yml

@dmartinol dmartinol left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally speaking, a great job in terms of problem analysis and 360-degree solution.
anyway, since we're planning to move the skill content to a different repo, or maybe even to separate repos, I'd avoid adding new cross-packs validations.
can we keep the Makefile and script changes for another run and just take the minimal changes to ensure consistent definition of the skills?
roughly, this means to keep all files apart from those under .cursor/, .github/, scripts/, specs/ and the Makefile.

@RoddieKieley

Copy link
Copy Markdown
Contributor Author

anyway, since we're planning to move the skill content to a different repo, or maybe even to separate repos, I'd avoid adding new cross-packs validations.

ack

can we keep the Makefile and script changes for another run and just take the minimal changes to ensure consistent definition of the skills?

Yes, should be able to split up the work with the automated script updates separated from the actual content updates.

roughly, this means to keep all files apart from those under .cursor/, .github/, scripts/, specs/ and the Makefile.

Good guidance, will check.

Revert .cursor/.github/scripts/specs/Makefile changes to origin/main and keep only pack/docs metadata and skill-definition consistency updates.
@RoddieKieley
RoddieKieley requested a review from dmartinol May 6, 2026 15:39
@RoddieKieley

Copy link
Copy Markdown
Contributor Author

@dmartinol Hopefully this is reduced to the valuable pieces that are able to be committed so that we can complete this PR #63 . I simply added another commit with the reduction, so the branch history should have the other content available when required.

@dmartinol dmartinol left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I will remove extra references to make validate-consistency-audit later

@dmartinol
dmartinol merged commit 40fa486 into RHEcosystemAppEng:main May 7, 2026
4 checks passed
@dmartinol dmartinol mentioned this pull request May 7, 2026
3 tasks
dmartinol added a commit that referenced this pull request May 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants