HYPERFLEET-909: fix drift and remove hardcoded values from standards-audit and review-pr#28
Merged
openshift-merge-bot[bot] merged 2 commits intoopenshift-hyperfleet:mainfrom Apr 9, 2026
Conversation
WalkthroughThis PR updates two plugin manifests (hyperfleet-code-review 0.3.0 → 0.4.1; hyperfleet-standards 1.0.0 → 1.1.0); deletes mechanical-passes.md and adds ten per-group mechanical check documents (group-01…group-10); refactors the review-pr skill to consolidate data gathering, insert a dedicated batch standards-fetch step using gh, renumber downstream steps, and make mechanical checks conditional on fetched standards and on the presence of Sequence Diagram(s)sequenceDiagram
participant Agent as Reviewer Agent
participant CR as hyperfleet-code-review Skill
participant GH as gh CLI / GitHub API
participant Repo as Standards Repo
Agent->>CR: Start PR review (gather data, validate ticket)
CR->>GH: Batch-fetch hyperfleet/standards/*.md
GH->>Repo: Request standard files
Repo-->>GH: Return files (content or errors)
GH-->>CR: Deliver fetched standards (content + status)
CR->>CR: Orchestrate per-group mechanical checks (group-01..group-10)
alt No .go files
CR->>CR: Skip Go-specific groups (1–7, 10)
end
CR-->>Agent: Emit findings (include standards-unavailable notes when applicable)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
hyperfleet-standards/skills/standards-audit/references/helm-chart-checks.md (2)
128-128: Consider refining the AppVersion search to reduce false positives.The current grep will flag all uses of
.Chart.AppVersion, including the legitimate fallback for theapp.kubernetes.io/versionlabel mentioned in Check 9. To focus on image tag issues, consider excluding the label helper or adding context to help distinguish legitimate uses.🔍 Suggested refinement
# Check image.tag does NOT fall back to .Chart.AppVersion in templates -grep -n "\.Chart\.AppVersion" charts/*/templates/*.yaml charts/*/templates/_helpers.tpl 2>/dev/null +# Exclude version label helper, focus on image references +grep -n "\.Chart\.AppVersion" charts/*/templates/*.yaml charts/*/templates/_helpers.tpl 2>/dev/null | grep -v "app.kubernetes.io/version"Alternatively, document that matches in version labels are expected and should be ignored during manual review.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet-standards/skills/standards-audit/references/helm-chart-checks.md` at line 128, The grep search for ".Chart.AppVersion" is too broad and produces false positives (it matches legitimate uses such as the fallback in the app.kubernetes.io/version label helper); narrow the search to target image tag usage by excluding the label helper file or adding contextual anchors: modify the command to exclude templates/_helpers.tpl (or files containing the "app.kubernetes.io/version" helper) or require surrounding text like "image:" or ".Values.image" when matching ".Chart.AppVersion"; update references to ".Chart.AppVersion" and "app.kubernetes.io/version" in the check description so reviewers know to ignore expected matches in the label helper.
151-153: Consider enhancing the naming convention verification command.The current command searches for any occurrence of "broker" in config maps but doesn't effectively verify the camelCase (values.yaml) vs snake_case (generated config) requirement. A more targeted approach could help reviewers identify naming convention issues more quickly.
🔧 Suggested enhancement
-# Check camelCase in values vs snake_case in generated config -grep -n "broker" charts/*/templates/configmap*.yaml 2>/dev/null +# Check camelCase in values.yaml broker keys +grep -A20 "^broker:" charts/*/values.yaml 2>/dev/null | grep -n "[a-z]" | head -10 + +# Check snake_case in generated broker config (look for broker_ patterns) +grep -n "broker\." charts/*/templates/configmap*.yaml 2>/dev/null +grep -n "broker_" charts/*/templates/configmap*.yaml 2>/dev/nullThis provides separate inspection points for values.yaml keys and generated config patterns, though manual review remains necessary to fully verify the naming conventions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet-standards/skills/standards-audit/references/helm-chart-checks.md` around lines 151 - 153, The current generic grep in helm-chart-checks.md should be replaced with two targeted checks: search charts/*/values.yaml for keys that use camelCase and include "broker" to catch values.yaml naming, and separately search charts/*/templates/configmap*.yaml for snake_case occurrences of "broker" to catch generated config patterns; implement each as a regex-based grep (with -nE) so reviewers get line numbers and clear matches, making it easy to spot mismatches between values.yaml keys and generated config map names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@hyperfleet-standards/skills/standards-audit/references/helm-chart-checks.md`:
- Line 128: The grep search for ".Chart.AppVersion" is too broad and produces
false positives (it matches legitimate uses such as the fallback in the
app.kubernetes.io/version label helper); narrow the search to target image tag
usage by excluding the label helper file or adding contextual anchors: modify
the command to exclude templates/_helpers.tpl (or files containing the
"app.kubernetes.io/version" helper) or require surrounding text like "image:" or
".Values.image" when matching ".Chart.AppVersion"; update references to
".Chart.AppVersion" and "app.kubernetes.io/version" in the check description so
reviewers know to ignore expected matches in the label helper.
- Around line 151-153: The current generic grep in helm-chart-checks.md should
be replaced with two targeted checks: search charts/*/values.yaml for keys that
use camelCase and include "broker" to catch values.yaml naming, and separately
search charts/*/templates/configmap*.yaml for snake_case occurrences of "broker"
to catch generated config patterns; implement each as a regex-based grep (with
-nE) so reviewers get line numbers and clear matches, making it easy to spot
mismatches between values.yaml keys and generated config map names.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c649afc-53f0-44d3-97b8-ab3f2cd78005
📒 Files selected for processing (9)
hyperfleet-code-review/.claude-plugin/plugin.jsonhyperfleet-code-review/README.mdhyperfleet-code-review/skills/review-pr/SKILL.mdhyperfleet-code-review/skills/review-pr/mechanical-passes.mdhyperfleet-standards/.claude-plugin/plugin.jsonhyperfleet-standards/README.mdhyperfleet-standards/skills/standards-audit/SKILL.mdhyperfleet-standards/skills/standards-audit/references/container-image-checks.mdhyperfleet-standards/skills/standards-audit/references/helm-chart-checks.md
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hyperfleet-standards/skills/standards-audit/references/helm-chart-checks.md`:
- Around line 124-126: The current check that runs "head -5
charts/*/templates/deployment.yaml" is brittle and may miss valid charts where
the include of validateValues is pushed down by comments; update the check to
search the deployment.yaml templates for an include of the validateValues
template symbolically (e.g., look for '{{ include "validateValues"' or '{{-
include "validateValues"' across the whole file) instead of relying on line
numbers, and ensure the validation script examines every
charts/*/templates/deployment.yaml file for that include pattern.
- Around line 147-155: The current checks miss distant `type` keys and only
detect specific inferred broker subkeys and generic `broker` mentions; update
the checks so: (1) replace the fragile grep "^broker:" + "type:" pattern with a
tool-aware search (e.g., yq or multiline grep) that ensures a `type` key exists
anywhere under the `broker` mapping (look for YAML path `broker.type`), (2)
replace the hardcoded subkey grep for
`.Values.broker.googlepubsub|.Values.broker.rabbitmq` with a generic regex that
detects any `.Values.broker.<subkey>` usage (e.g., search for
`.Values.broker\.[A-Za-z0-9_]+` in templates to catch inferred-type logic), and
(3) tighten the camelCase vs snake_case check by verifying specific mappings in
configmap templates (search for camelCase keys like `broker[A-Z]` and
corresponding snake_case `broker_[a-z]` occurrences or assert a transformation
rule rather than merely presence of the word `broker` in
templates/configmap*.yaml).
- Around line 127-131: The grep filter flags any occurrence of
".Chart.AppVersion" even when it's used only inside the allowed labels helper
(app.kubernetes.io/version) across templates/_helpers.tpl; update the check so
it only reports matches that appear in image lines or image-related helpers:
restrict the search to files and lines that contain "image:" or
functions/helpers with "image" in their name (e.g., templates/*/templates/*.yaml
and _helpers.tpl but only when the matched line or surrounding helper contains
"image"), and exclude matches that are inside the labels helper that sets
"app.kubernetes.io/version" (keep the current exclusion for
"app.kubernetes.io/version" but also ignore matches found within the labels
helper block in _helpers.tpl).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f40f7146-2e10-448d-944d-5ab32aa4cf39
📒 Files selected for processing (9)
hyperfleet-code-review/.claude-plugin/plugin.jsonhyperfleet-code-review/README.mdhyperfleet-code-review/skills/review-pr/SKILL.mdhyperfleet-code-review/skills/review-pr/mechanical-passes.mdhyperfleet-standards/.claude-plugin/plugin.jsonhyperfleet-standards/README.mdhyperfleet-standards/skills/standards-audit/SKILL.mdhyperfleet-standards/skills/standards-audit/references/container-image-checks.mdhyperfleet-standards/skills/standards-audit/references/helm-chart-checks.md
✅ Files skipped from review due to trivial changes (4)
- hyperfleet-code-review/README.md
- hyperfleet-code-review/.claude-plugin/plugin.json
- hyperfleet-standards/.claude-plugin/plugin.json
- hyperfleet-code-review/skills/review-pr/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (4)
- hyperfleet-standards/skills/standards-audit/references/container-image-checks.md
- hyperfleet-standards/README.md
- hyperfleet-standards/skills/standards-audit/SKILL.md
- hyperfleet-code-review/skills/review-pr/mechanical-passes.md
hyperfleet-standards/skills/standards-audit/references/helm-chart-checks.md
Outdated
Show resolved
Hide resolved
hyperfleet-standards/skills/standards-audit/references/helm-chart-conventions-checks.md
Outdated
Show resolved
Hide resolved
hyperfleet-standards/skills/standards-audit/references/helm-chart-checks.md
Outdated
Show resolved
Hide resolved
rafabene
changed the title
rafabene
force-pushed
the
HYPERFLEET-909
branch
2 times, most recently
from
20cdb2a to
bf5a1f5
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
hyperfleet-standards/skills/standards-audit/SKILL.md (2)
188-210:⚠️ Potential issue | 🟠 MajorWorkflow instructions conflict on standards source.
This section still says agents receive content fetched “via hyperfleet-architecture skill,” but the updated discovery flow fetches standards directly via
gh api. Keep one canonical source path, otherwise execution and debugging become ambiguous.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet-standards/skills/standards-audit/SKILL.md` around lines 188 - 210, The SKILL.md workflow contradicts itself by saying agents receive standard document content "via hyperfleet-architecture skill" while the discovery flow now fetches standards via `gh api`; pick one canonical source (either the `hyperfleet-architecture` skill or `gh api`) and update all references in the "Launch one agent..." paragraph and any related instructions so they consistently point to that single source; ensure the agent input list (the block mentioning `subagent_type=general-purpose`) and the numbered agent requirements (steps 1–3) reference the chosen source only, and remove or replace the alternate path (`hyperfleet-architecture skill` or `gh api`) everywhere in SKILL.md to avoid ambiguity.
321-322:⚠️ Potential issue | 🟠 MajorError handling no longer matches the updated fetch mechanism.
The failure path still attributes standards fetch errors to
hyperfleet-architectureskill, but fetch now occurs viagh api. Update this to report GH/API/auth failures explicitly, and reserve architecture-skill errors for drift detection invocation only.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet-standards/skills/standards-audit/SKILL.md` around lines 321 - 322, Update the failure messaging in the two bullets so they reflect the new fetch path via gh api: replace references that blame the "hyperfleet-architecture skill" for standards fetch failures with specific GH/API/auth error wording (e.g., "Failed to fetch standards via GitHub API — check GH token, repo access, and API availability"), and keep mentions of "hyperfleet-architecture" limited to drift-detection invocation failures only (e.g., "hyperfleet-architecture plugin invocation failed" and link install docs). Ensure the first bullet reports which standards couldn't be fetched and suggests verifying GitHub auth/repo access, and the second bullet explicitly states the architecture plugin is not installed and links to installation instructions.
🧹 Nitpick comments (3)
hyperfleet-standards/skills/standards-audit/references/tracing-checks.md (1)
154-156: Context-propagation grep is likely to over-report valid initialization code.Line 155 only excludes
_test.goandmain.go; initialization often lives ininit.go, bootstrap packages, or wiring layers. This can create noisy findings and dilute real call-chain propagation issues.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet-standards/skills/standards-audit/references/tracing-checks.md` around lines 154 - 156, The grep rule that searches for "context.Background()|context.TODO()" currently only excludes "_test.go" and "main.go", causing noisy matches in legitimate initialization files; update the command (the grep pipeline shown) to also exclude common init/bootstrap/wiring files and directories such as "init.go", "init_*.go", "bootstrap*.go", "wire.go", and wiring/boot packages (or patterns like "*/bootstrap/*" and "*/wire/*") so the check ignores these known initialization locations while still catching inappropriate usage elsewhere.hyperfleet-standards/skills/standards-audit/references/health-endpoints-checks.md (1)
78-80: Preserve file/line context in the SIGTERM/readiness check command.Line 79 drops line-number context (
-n) and then filters through a secondgrep, which makes findings harder to trace back to source quickly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet-standards/skills/standards-audit/references/health-endpoints-checks.md` around lines 78 - 80, The current grep pipeline that searches for "SIGTERM\|signal.Notify" and then pipes to a second grep for "ready\|readyz" loses file/line number context; update the command to preserve line numbers by adding -n (or --line-number) to the first grep and avoid removing it in the second filter (e.g., use grep -n -B5 -A10 "SIGTERM\|signal.Notify" --include="*.go" 2>/dev/null | grep -n -i "ready\|readyz" | head -5 or combine into a single grep/awk that reports filenames and line numbers), ensuring references to SIGTERM/signal.Notify and ready/readyz remain in the pipeline so results include file:line for easy tracing.hyperfleet-standards/skills/standards-audit/references/logging-checks.md (1)
114-123: Broaden package discovery to avoid false negatives in shared-library checks.Lines 116–123 assume shared code always lives under
pkg/orinternal/. Repos with other layouts (for example,lib/or nested modules) can be misclassified as compliant. Consider using a file-first scan strategy instead of fixed roots.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet-standards/skills/standards-audit/references/logging-checks.md` around lines 114 - 123, The current checks rely on fixed roots (the ls and grep calls: "ls pkg/common/ pkg/shared/ internal/common/ internal/shared/" and the subsequent grep over pkg/ and internal/) which misses repos that place shared code in other dirs like lib/ or nested modules; change the discovery to a file-first scan (e.g., enumerate repository files via find or git ls-files) and then run the existing grep patterns ("slog.New|slog.Default|zap.New|zerolog.New" and "func.*context.Context|func.*\*slog.Logger|func.*\*zap.Logger") against that file list instead of hardcoding pkg/ and internal/, so repos with lib/ or other layouts are correctly analyzed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hyperfleet-architecture/skills/standards-drift-detector/SKILL.md`:
- Around line 75-82: The current normative extraction step only matches
uppercase RFC2119 keywords and will miss lowercase/mixed-case occurrences;
update the normative-requirement extraction logic (the "For each standard..." /
RFC 2119 keywords step) to perform case-insensitive matching for the keywords
MUST, MUST NOT, SHALL, SHALL NOT, and REQUIRED (and their surrounding
word-boundary variants) so sentences containing "must", "Must", "shall", etc.
are included; ensure the extractor uses whole-word, case-insensitive regex or a
case-normalizing pass before keyword detection to avoid false negatives.
In
`@hyperfleet-standards/skills/standards-audit/references/configuration-checks.md`:
- Around line 95-102: The current YAML enforcement only inspects a few hardcoded
paths and truncates output with head -30, so update the check to: search the
repository for any config files with JSON/TOML extensions or files whose content
type is non-YAML (use find with -iname '*.json' -o -iname '*.toml' and/or file
--mime-type to detect non-yaml), and list full contents for inspection (remove
head -30); also include scanning for .yml/.yaml candidates in any directories
(not just configs/) to validate property names across all discovered YAML files;
replace the hardcoded ls/cat commands in the Check 5 snippet with these broader
find+file inspections to avoid false passes.
In
`@hyperfleet-standards/skills/standards-audit/references/dependency-pinning-checks.md`:
- Around line 83-91: The verification is underspecified because the grep
alternation can false-pass; update the guidance in
standards-audit/references/dependency-pinning-checks.md to explicitly require
asserting the presence of all three Makefile targets (tools-install, tools-list,
tools-update) and their corresponding .PHONY declarations; replace the single
alternation grep examples with explicit checks that verify each target exists
and each .PHONY line contains all three names (or recommend three individual
greps/tests) and show clear commands or regexes referencing the exact target
names (tools-install, tools-list, tools-update) and the literal ".PHONY" token
so reviewers/tools can reliably fail when any one of the required entries is
missing.
In
`@hyperfleet-standards/skills/standards-audit/references/error-model-checks.md`:
- Around line 49-50: The doc currently requires the `type` field to MUST be a
URI but shows non-URI examples like `validation-error`, causing a contradiction;
update the requirement and examples so they agree: change the text around `type`
to require a registered problem type URI (e.g., use full URIs instead of bare
tokens) and replace example values such as `validation-error`,
`authentication-required`, `resource-not-found` with canonical URI examples (for
instance `https://your.api/problems/validation-error`) so the spec and examples
for `type` are consistent; ensure the `timestamp` RFC 3339 and
`application/problem+json` Content-Type guidance remain unchanged.
In
`@hyperfleet-standards/skills/standards-audit/references/graceful-shutdown-checks.md`:
- Around line 92-101: Update the shutdown-tests verification steps to also
detect assertions around process exit semantics by adding a search for os.Exit
and related exit/status patterns; specifically, append a grep that looks for
"os.Exit", "ExitCode", "wait.*status", and "exec.ExitError" (in addition to the
existing checks for
SIGTERM/SIGINT/TestShutdown/TestGraceful/TestSignal/TestDrain/TestTimeout) so
the docs can verify "clean exit code" is tested (look for these symbols in test
files: os.Exit, ExitCode, wait.*status, exec.ExitError, TestShutdown).
In `@hyperfleet-standards/skills/standards-audit/references/makefile-checks.md`:
- Around line 119-128: The current verification only checks for target
existence; change the check to assert that the `container-build` and
`container-push` targets list `check-container-tool` as a prerequisite. Update
the instructions/commands that reference `check-container-tool`,
`container-build`, and `container-push` so they parse the Makefile rule line
(e.g., look for a line matching `^container-build:.*\bcheck-container-tool\b`
and similarly for `container-push`) or use `make -pn` to inspect target
prerequisites and fail if `check-container-tool` is not present in either
target's prerequisites.
- Around line 73-85: The current check uses grep for any of BUILD_DATE, GIT_SHA,
GIT_DIRTY, APP_VERSION which allows partial matches; change the verification to
ensure all four variables are present by replacing the loose grep with a
count-based check (e.g., grep -E "BUILD_DATE|GIT_SHA|GIT_DIRTY|APP_VERSION"
Makefile and ensure the match count equals 4) and update the incorrect-VERSION
test to explicitly detect "^VERSION\s*[:?]\?=" as before; reference the symbols
BUILD_DATE, GIT_SHA, GIT_DIRTY, APP_VERSION and the existing grep lines in the
Makefile-checks.md content when making this change.
---
Outside diff comments:
In `@hyperfleet-standards/skills/standards-audit/SKILL.md`:
- Around line 188-210: The SKILL.md workflow contradicts itself by saying agents
receive standard document content "via hyperfleet-architecture skill" while the
discovery flow now fetches standards via `gh api`; pick one canonical source
(either the `hyperfleet-architecture` skill or `gh api`) and update all
references in the "Launch one agent..." paragraph and any related instructions
so they consistently point to that single source; ensure the agent input list
(the block mentioning `subagent_type=general-purpose`) and the numbered agent
requirements (steps 1–3) reference the chosen source only, and remove or replace
the alternate path (`hyperfleet-architecture skill` or `gh api`) everywhere in
SKILL.md to avoid ambiguity.
- Around line 321-322: Update the failure messaging in the two bullets so they
reflect the new fetch path via gh api: replace references that blame the
"hyperfleet-architecture skill" for standards fetch failures with specific
GH/API/auth error wording (e.g., "Failed to fetch standards via GitHub API —
check GH token, repo access, and API availability"), and keep mentions of
"hyperfleet-architecture" limited to drift-detection invocation failures only
(e.g., "hyperfleet-architecture plugin invocation failed" and link install
docs). Ensure the first bullet reports which standards couldn't be fetched and
suggests verifying GitHub auth/repo access, and the second bullet explicitly
states the architecture plugin is not installed and links to installation
instructions.
---
Nitpick comments:
In
`@hyperfleet-standards/skills/standards-audit/references/health-endpoints-checks.md`:
- Around line 78-80: The current grep pipeline that searches for
"SIGTERM\|signal.Notify" and then pipes to a second grep for "ready\|readyz"
loses file/line number context; update the command to preserve line numbers by
adding -n (or --line-number) to the first grep and avoid removing it in the
second filter (e.g., use grep -n -B5 -A10 "SIGTERM\|signal.Notify"
--include="*.go" 2>/dev/null | grep -n -i "ready\|readyz" | head -5 or combine
into a single grep/awk that reports filenames and line numbers), ensuring
references to SIGTERM/signal.Notify and ready/readyz remain in the pipeline so
results include file:line for easy tracing.
In `@hyperfleet-standards/skills/standards-audit/references/logging-checks.md`:
- Around line 114-123: The current checks rely on fixed roots (the ls and grep
calls: "ls pkg/common/ pkg/shared/ internal/common/ internal/shared/" and the
subsequent grep over pkg/ and internal/) which misses repos that place shared
code in other dirs like lib/ or nested modules; change the discovery to a
file-first scan (e.g., enumerate repository files via find or git ls-files) and
then run the existing grep patterns ("slog.New|slog.Default|zap.New|zerolog.New"
and "func.*context.Context|func.*\*slog.Logger|func.*\*zap.Logger") against that
file list instead of hardcoding pkg/ and internal/, so repos with lib/ or other
layouts are correctly analyzed.
In `@hyperfleet-standards/skills/standards-audit/references/tracing-checks.md`:
- Around line 154-156: The grep rule that searches for
"context.Background()|context.TODO()" currently only excludes "_test.go" and
"main.go", causing noisy matches in legitimate initialization files; update the
command (the grep pipeline shown) to also exclude common init/bootstrap/wiring
files and directories such as "init.go", "init_*.go", "bootstrap*.go",
"wire.go", and wiring/boot packages (or patterns like "*/bootstrap/*" and
"*/wire/*") so the check ignores these known initialization locations while
still catching inappropriate usage elsewhere.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 394dd286-5875-4203-abc1-7be037c4ef50
📒 Files selected for processing (23)
CLAUDE.mdhyperfleet-architecture/.claude-plugin/plugin.jsonhyperfleet-architecture/README.mdhyperfleet-architecture/skills/standards-drift-detector/SKILL.mdhyperfleet-code-review/.claude-plugin/plugin.jsonhyperfleet-code-review/README.mdhyperfleet-code-review/skills/review-pr/SKILL.mdhyperfleet-code-review/skills/review-pr/mechanical-passes.mdhyperfleet-standards/.claude-plugin/plugin.jsonhyperfleet-standards/README.mdhyperfleet-standards/skills/standards-audit/SKILL.mdhyperfleet-standards/skills/standards-audit/references/configuration-checks.mdhyperfleet-standards/skills/standards-audit/references/container-image-checks.mdhyperfleet-standards/skills/standards-audit/references/dependency-pinning-checks.mdhyperfleet-standards/skills/standards-audit/references/directory-structure-checks.mdhyperfleet-standards/skills/standards-audit/references/error-model-checks.mdhyperfleet-standards/skills/standards-audit/references/graceful-shutdown-checks.mdhyperfleet-standards/skills/standards-audit/references/health-endpoints-checks.mdhyperfleet-standards/skills/standards-audit/references/helm-chart-checks.mdhyperfleet-standards/skills/standards-audit/references/linting-checks.mdhyperfleet-standards/skills/standards-audit/references/logging-checks.mdhyperfleet-standards/skills/standards-audit/references/makefile-checks.mdhyperfleet-standards/skills/standards-audit/references/tracing-checks.md
✅ Files skipped from review due to trivial changes (8)
- hyperfleet-code-review/.claude-plugin/plugin.json
- hyperfleet-architecture/.claude-plugin/plugin.json
- hyperfleet-code-review/README.md
- hyperfleet-standards/.claude-plugin/plugin.json
- hyperfleet-architecture/README.md
- hyperfleet-standards/skills/standards-audit/references/directory-structure-checks.md
- hyperfleet-standards/skills/standards-audit/references/helm-chart-checks.md
- hyperfleet-standards/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- hyperfleet-standards/skills/standards-audit/references/container-image-checks.md
- hyperfleet-code-review/skills/review-pr/SKILL.md
- hyperfleet-code-review/skills/review-pr/mechanical-passes.md
hyperfleet-architecture/skills/standards-drift-detector/SKILL.md
Outdated
Show resolved
Hide resolved
hyperfleet-standards/skills/standards-audit/references/configuration-checks.md
Outdated
Show resolved
Hide resolved
hyperfleet-standards/skills/standards-audit/references/dependency-pinning-checks.md
Outdated
Show resolved
Hide resolved
hyperfleet-standards/skills/standards-audit/references/error-model-checks.md
Outdated
Show resolved
Hide resolved
hyperfleet-standards/skills/standards-audit/references/graceful-shutdown-checks.md
Outdated
Show resolved
Hide resolved
hyperfleet-standards/skills/standards-audit/references/makefile-conventions-checks.md
Outdated
Show resolved
Hide resolved
hyperfleet-standards/skills/standards-audit/references/makefile-conventions-checks.md
Outdated
Show resolved
Hide resolved
rafabene
force-pushed
the
HYPERFLEET-909
branch
2 times, most recently
from
4a7074d to
c2e5ad7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
hyperfleet-standards/skills/standards-audit/references/error-model-checks.md (1)
49-49:⚠️ Potential issue | 🟠 MajorThe
typeURI requirement still conflicts with the provided examples.Line 49 states the
typefield "MUST be a URI" but the examples (validation-error,authentication-required,resource-not-found) remain non-URI tokens. This contradiction persists from the previous review and will cause incorrect audit outcomes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet-standards/skills/standards-audit/references/error-model-checks.md` at line 49, The doc's requirement for the error `type` field to "MUST be a URI" conflicts with the example tokens; update the text and examples so they match: change the examples `validation-error`, `authentication-required`, `resource-not-found` into full URIs (e.g., a canonical registry URI) or relax the requirement to allow registered tokens, and make the chosen approach consistent in the sentence that mentions `type` and in the examples block; ensure the `type` rule, example values, and any mention of "registered problem types list" all reflect the same format (URI or token) so the audit behavior for `type` is unambiguous.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hyperfleet-architecture/skills/standards-drift-detector/SKILL.md`:
- Around line 69-71: The file-mode matching logic described in the "If path is a
file" section is too strict—it's only scanning for literal filenames like
`error-model.md` and `logging-specification.md` and thus misses descriptive
references (e.g., "error model standard", "logging specification") found in
files like `mechanical-passes.md`; update the detector to match both literal
filenames AND descriptive names by extending the search patterns to include
common human-readable variants (e.g., "error model", "error model standard",
"logging specification") and normalize/mapping any matched descriptive name to
the canonical standard filename (e.g., map "error model standard" ->
`error-model.md`) so that section extraction (used as check definitions)
includes sections that reference either form.
In `@hyperfleet-standards/skills/standards-audit/SKILL.md`:
- Around line 38-56: The loop uses placeholder syntax "for file in
<file_names_from_step_1>" which is not valid bash; replace the two-step
instructions with a single executable bash loop that captures filenames by
command substitution from the gh api call and iterates over them (i.e., use for
file in $(gh api
repos/openshift-hyperfleet/architecture/contents/hyperfleet/standards --jq '.[]
| select(.name | endswith(".md")) | .name' 2>/dev/null)); do ... done), include
a shebang (#!/bin/bash) at the top, call the per-file gh api to fetch .content
and base64 --decode inside the loop, and handle errors (e.g., append || echo
"Warning: Failed to fetch $file") so SKILL.md’s Step 2 is executable end-to-end.
---
Duplicate comments:
In
`@hyperfleet-standards/skills/standards-audit/references/error-model-checks.md`:
- Line 49: The doc's requirement for the error `type` field to "MUST be a URI"
conflicts with the example tokens; update the text and examples so they match:
change the examples `validation-error`, `authentication-required`,
`resource-not-found` into full URIs (e.g., a canonical registry URI) or relax
the requirement to allow registered tokens, and make the chosen approach
consistent in the sentence that mentions `type` and in the examples block;
ensure the `type` rule, example values, and any mention of "registered problem
types list" all reflect the same format (URI or token) so the audit behavior for
`type` is unambiguous.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c2bd11fc-f5a1-471d-91f3-c63b0486c1a0
📒 Files selected for processing (23)
CLAUDE.mdhyperfleet-architecture/.claude-plugin/plugin.jsonhyperfleet-architecture/README.mdhyperfleet-architecture/skills/standards-drift-detector/SKILL.mdhyperfleet-code-review/.claude-plugin/plugin.jsonhyperfleet-code-review/README.mdhyperfleet-code-review/skills/review-pr/SKILL.mdhyperfleet-code-review/skills/review-pr/mechanical-passes.mdhyperfleet-standards/.claude-plugin/plugin.jsonhyperfleet-standards/README.mdhyperfleet-standards/skills/standards-audit/SKILL.mdhyperfleet-standards/skills/standards-audit/references/configuration-checks.mdhyperfleet-standards/skills/standards-audit/references/container-image-checks.mdhyperfleet-standards/skills/standards-audit/references/dependency-pinning-checks.mdhyperfleet-standards/skills/standards-audit/references/directory-structure-checks.mdhyperfleet-standards/skills/standards-audit/references/error-model-checks.mdhyperfleet-standards/skills/standards-audit/references/graceful-shutdown-checks.mdhyperfleet-standards/skills/standards-audit/references/health-endpoints-checks.mdhyperfleet-standards/skills/standards-audit/references/helm-chart-checks.mdhyperfleet-standards/skills/standards-audit/references/linting-checks.mdhyperfleet-standards/skills/standards-audit/references/logging-checks.mdhyperfleet-standards/skills/standards-audit/references/makefile-checks.mdhyperfleet-standards/skills/standards-audit/references/tracing-checks.md
✅ Files skipped from review due to trivial changes (9)
- hyperfleet-standards/.claude-plugin/plugin.json
- hyperfleet-code-review/.claude-plugin/plugin.json
- hyperfleet-standards/skills/standards-audit/references/dependency-pinning-checks.md
- hyperfleet-code-review/README.md
- hyperfleet-architecture/.claude-plugin/plugin.json
- hyperfleet-standards/skills/standards-audit/references/linting-checks.md
- hyperfleet-architecture/README.md
- CLAUDE.md
- hyperfleet-standards/skills/standards-audit/references/directory-structure-checks.md
🚧 Files skipped from review as they are similar to previous changes (4)
- hyperfleet-standards/skills/standards-audit/references/graceful-shutdown-checks.md
- hyperfleet-standards/skills/standards-audit/references/makefile-checks.md
- hyperfleet-code-review/skills/review-pr/SKILL.md
- hyperfleet-code-review/skills/review-pr/mechanical-passes.md
hyperfleet-architecture/skills/standards-drift-detector/SKILL.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
hyperfleet-standards/skills/standards-audit/references/helm-chart-checks.md (1)
159-163:⚠️ Potential issue | 🟠 Major
broker.typefallback check is still brittle and can miss valid/invalid cases.When
yqis unavailable,grep -A20 "^broker:" ... | grep "type:"only scans 20 lines underbroker:. Deeply nested or longer blocks can produce false results.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet-standards/skills/standards-audit/references/helm-chart-checks.md` around lines 159 - 163, The fallback grep is brittle because it only grabs 20 lines after a broker: match; replace the fallback pipeline that currently uses grep -A20 "^broker:" ... | grep "type:" with an awk-based extraction that prints the entire broker mapping block before searching for type (e.g. use awk '/^broker:/{flag=1} flag{print} /^([[:alnum:]_-]+):/{if(flag && $0!~/^broker:/) exit}' charts/*/values.yaml | grep "type:"), so update the line that invokes the grep fallback to use an awk block extractor + grep for "type:" to reliably find broker.type in deeply nested or long mappings.hyperfleet-standards/skills/standards-drift-detector/SKILL.md (1)
98-102:⚠️ Potential issue | 🟠 MajorOutput contract is still internally inconsistent for partial fetch scenarios.
Line 98–102 mandates exact output
No drift detected., but Line 128 says fetch failures must be noted in output. When fetch partially fails with zero uncovered requirements, both rules cannot be satisfied simultaneously. Define one deterministic “no drift + warnings” format.Also applies to: 128-129
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet-standards/skills/standards-drift-detector/SKILL.md` around lines 98 - 102, The output contract is inconsistent: the rule requiring exact output "No drift detected." conflicts with the rule that fetch failures must be noted when a partial fetch returns zero uncovered requirements; update the SKILL.md spec to define a single deterministic format for this case (e.g., require "No drift detected." followed by a standardized optional warnings section when fetch failures occurred, such as "Warnings: <fetch failure summary>" or a structured "No drift detected. Warnings: ..." line). Modify the paragraphs referencing the exact string (the rule that currently says Output exactly: `No drift detected.`) and the fetch-failure note (the rules around fetch failures) so they reference this unified format and give the exact expected text/structure for automated checks.hyperfleet-standards/skills/standards-audit/references/configuration-checks.md (1)
99-107:⚠️ Potential issue | 🟠 MajorYAML-format enforcement is still incomplete and can miss violations.
This still risks false PASS: discovery is limited to paths matching
*/config*(Line 100/103), and property inspection only reads two hardcoded files (Line 106), ignoring other discovered YAML configs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet-standards/skills/standards-audit/references/configuration-checks.md` around lines 99 - 107, The YAML discovery and checking are incomplete: replace the two hardcoded find/cat usages so all discovered YAML config files are inspected instead of only files under "*/config*" and the two hardcoded files; capture YAMLs with the existing find (use -print0 to handle spaces) and then iterate over the resulting paths (e.g., via xargs -0 or a while read -r loop) to run the property-name inspection for each file; specifically update the find command(s) referenced in the diff and remove/replace the "cat configs/config.yaml config/config.yaml" invocation so the inspection consumes the full list of discovered files rather than two hardcoded paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@hyperfleet-standards/skills/standards-audit/references/configuration-checks.md`:
- Around line 79-83: The grep invocations using patterns like grep -rn
"StringVar\|IntVar\|BoolVar\|Flags()\|PersistentFlags" --include="*.go" and grep
-rn "\"name\"\|\"n\".*name\|MarkFlagRequired.*name" --include="*.go" are missing
a search root and may read stdin; update each such grep command to include a
search path (e.g., add . after the --include argument) so they scan the
repository (replace occurrences of grep -rn ... --include="*.go" with grep -rn
... --include="*.go" .) ensuring all instances are fixed.
In
`@hyperfleet-standards/skills/standards-audit/references/directory-structure-checks.md`:
- Line 107: The grep invocation shown (grep -rn "go generate\|//go:generate"
--include="*.go") lacks a search path and can hang or miss files; update the
command used in this check to include an explicit path (e.g., add '.' as the
final argument) so the repository is actually scanned—locate the grep call in
directory-structure-checks.md and add the path operand to the command string.
In `@hyperfleet-standards/skills/standards-audit/references/helm-chart-checks.md`:
- Around line 114-115: The check text is inconsistent about where the
`validateValues` guard must be invoked: update the rule to be consistent by
either (A) enforcing invocation at the top of `deployment.yaml` — change the
how-to/find command to specifically search the top of `deployment.yaml` for the
`include ".*validateValues.*"` call and keep the requirement text as-is, or (B)
relax the requirement text to say the guard must be invoked anywhere in
`deployment.yaml` — update the sentence on Line 114 (and related lines 124–126)
to match the existing command that looks for the inclusion anywhere; reference
the `validateValues` helper in `_helpers.tpl`, `deployment.yaml`, `image.tag`,
and the prohibition against falling back to `.Chart.AppVersion` when making the
change.
---
Duplicate comments:
In
`@hyperfleet-standards/skills/standards-audit/references/configuration-checks.md`:
- Around line 99-107: The YAML discovery and checking are incomplete: replace
the two hardcoded find/cat usages so all discovered YAML config files are
inspected instead of only files under "*/config*" and the two hardcoded files;
capture YAMLs with the existing find (use -print0 to handle spaces) and then
iterate over the resulting paths (e.g., via xargs -0 or a while read -r loop) to
run the property-name inspection for each file; specifically update the find
command(s) referenced in the diff and remove/replace the "cat
configs/config.yaml config/config.yaml" invocation so the inspection consumes
the full list of discovered files rather than two hardcoded paths.
In `@hyperfleet-standards/skills/standards-audit/references/helm-chart-checks.md`:
- Around line 159-163: The fallback grep is brittle because it only grabs 20
lines after a broker: match; replace the fallback pipeline that currently uses
grep -A20 "^broker:" ... | grep "type:" with an awk-based extraction that prints
the entire broker mapping block before searching for type (e.g. use awk
'/^broker:/{flag=1} flag{print} /^([[:alnum:]_-]+):/{if(flag && $0!~/^broker:/)
exit}' charts/*/values.yaml | grep "type:"), so update the line that invokes the
grep fallback to use an awk block extractor + grep for "type:" to reliably find
broker.type in deeply nested or long mappings.
In `@hyperfleet-standards/skills/standards-drift-detector/SKILL.md`:
- Around line 98-102: The output contract is inconsistent: the rule requiring
exact output "No drift detected." conflicts with the rule that fetch failures
must be noted when a partial fetch returns zero uncovered requirements; update
the SKILL.md spec to define a single deterministic format for this case (e.g.,
require "No drift detected." followed by a standardized optional warnings
section when fetch failures occurred, such as "Warnings: <fetch failure
summary>" or a structured "No drift detected. Warnings: ..." line). Modify the
paragraphs referencing the exact string (the rule that currently says Output
exactly: `No drift detected.`) and the fetch-failure note (the rules around
fetch failures) so they reference this unified format and give the exact
expected text/structure for automated checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 143d2967-fb46-46af-8a6d-03a00d25d61a
📒 Files selected for processing (22)
CLAUDE.mdhyperfleet-architecture/README.mdhyperfleet-code-review/.claude-plugin/plugin.jsonhyperfleet-code-review/README.mdhyperfleet-code-review/skills/review-pr/SKILL.mdhyperfleet-code-review/skills/review-pr/mechanical-passes.mdhyperfleet-standards/.claude-plugin/plugin.jsonhyperfleet-standards/README.mdhyperfleet-standards/skills/standards-audit/SKILL.mdhyperfleet-standards/skills/standards-audit/references/configuration-checks.mdhyperfleet-standards/skills/standards-audit/references/container-image-checks.mdhyperfleet-standards/skills/standards-audit/references/dependency-pinning-checks.mdhyperfleet-standards/skills/standards-audit/references/directory-structure-checks.mdhyperfleet-standards/skills/standards-audit/references/error-model-checks.mdhyperfleet-standards/skills/standards-audit/references/graceful-shutdown-checks.mdhyperfleet-standards/skills/standards-audit/references/health-endpoints-checks.mdhyperfleet-standards/skills/standards-audit/references/helm-chart-checks.mdhyperfleet-standards/skills/standards-audit/references/linting-checks.mdhyperfleet-standards/skills/standards-audit/references/logging-checks.mdhyperfleet-standards/skills/standards-audit/references/makefile-checks.mdhyperfleet-standards/skills/standards-audit/references/tracing-checks.mdhyperfleet-standards/skills/standards-drift-detector/SKILL.md
✅ Files skipped from review due to trivial changes (5)
- hyperfleet-standards/.claude-plugin/plugin.json
- hyperfleet-code-review/README.md
- hyperfleet-code-review/.claude-plugin/plugin.json
- hyperfleet-architecture/README.md
- hyperfleet-standards/skills/standards-audit/references/linting-checks.md
🚧 Files skipped from review as they are similar to previous changes (10)
- CLAUDE.md
- hyperfleet-standards/skills/standards-audit/references/graceful-shutdown-checks.md
- hyperfleet-standards/skills/standards-audit/references/dependency-pinning-checks.md
- hyperfleet-standards/skills/standards-audit/references/container-image-checks.md
- hyperfleet-standards/skills/standards-audit/references/health-endpoints-checks.md
- hyperfleet-standards/README.md
- hyperfleet-standards/skills/standards-audit/SKILL.md
- hyperfleet-standards/skills/standards-audit/references/makefile-checks.md
- hyperfleet-code-review/skills/review-pr/SKILL.md
- hyperfleet-code-review/skills/review-pr/mechanical-passes.md
hyperfleet-standards/skills/standards-audit/references/configuration-checks.md
Outdated
Show resolved
Hide resolved
hyperfleet-standards/skills/standards-audit/references/directory-structure-checks.md
Outdated
Show resolved
Hide resolved
hyperfleet-standards/skills/standards-audit/references/helm-chart-checks.md
Outdated
Show resolved
Hide resolved
rafabene
force-pushed
the
HYPERFLEET-909
branch
11 times, most recently
from
b510c7e to
f5dae0c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hyperfleet-code-review/skills/review-pr/SKILL.md (1)
109-117:⚠️ Potential issue | 🟠 MajorStep 5 needs explicit fallback when standards fetch is unavailable.
This step mandates standards-based validation but doesn’t define behavior if step 4b was skipped. Add a deterministic fallback (e.g., run intra-PR consistency only + mandatory “standards unavailable” note) to prevent ambiguous execution paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet-code-review/skills/review-pr/SKILL.md` around lines 109 - 117, Step 5 currently assumes standards from step 4b are available; add a deterministic fallback so when the "standards fetch" (step 4b) returns empty/error the Step 5 routine switches to a defined behavior: run only intra-PR consistency checks, emit a mandatory "standards unavailable" warning/note in the step output, and mark full standards-based validation as skipped; ensure the Step 5 control flow explicitly tests for missing/erroneous standards and returns this explicit status so downstream logic behaves deterministically.
♻️ Duplicate comments (4)
hyperfleet-standards/skills/standards-audit/references/helm-chart-checks.md (1)
124-125:⚠️ Potential issue | 🟠 MajorBroker discovery command reintroduces hardcoded drift and under-validates the rule
At Line 125, the
grep -rn "broker\|kafka\|amqp\|nats\|mqtt"check hardcodes protocol terms and only scansvalues.yaml, so it can miss compliant/non-compliant broker implementations and does not validate the TLS/auth requirements stated at Line 124.Suggested update
-**How to find:** `grep -rn "broker\|kafka\|amqp\|nats\|mqtt" charts/*/values.yaml 2>/dev/null` +**How to find:** Review broker-related sections in `values.yaml` and corresponding template usage from Step 3; validate structure and TLS/auth fields against the standard document (no protocol-specific keyword assumptions).As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet-standards/skills/standards-audit/references/helm-chart-checks.md` around lines 124 - 125, The current grep -rn "broker\|kafka\|amqp\|nats\|mqtt" charts/*/values.yaml check hardcodes protocol terms and only scans values.yaml, so replace it with a YAML-aware discovery and validation: use recursive search across chart directories (e.g., using grep -R --include='values.yaml' -E 'broker|kafka|amqp|nats|mqtt' OR better, iterate charts/* and parse values.yaml with yq) to locate any broker config sections, then validate required fields (TLS keys like tls.enabled/tls.ca/tls.cert, auth keys such as auth.username/auth.password/auth.secretRef, and structured broker blocks) using yq queries to ensure presence and types; update the doc to show the yq examples and the grep pattern only as a fallback, and remove the hardcoded-only claim at the current grep line.hyperfleet-standards/skills/standards-audit/references/configuration-checks.md (3)
42-43:⚠️ Potential issue | 🟠 MajorCheck 2 command cannot reliably audit env-var convention and may miss files.
At Line 43,
grephas no search root (.), and the pattern only finds API usage (BindEnv,Getenv, etc.) rather than validating actual env-var names against the standard’s prefix/casing rules. This can produce false PASS results.Proposed fix
-# Environment variable prefix — search for the prefix defined in the standard -grep -rn "os.Getenv\|Setenv\|BindEnv\|AutomaticEnv\|SetEnvPrefix" --include="*.go" 2>/dev/null | head -20 +# Environment variable convention — collect env-var names, then compare with standard-required pattern +grep -rnE 'os\.Getenv\(|Setenv\(|BindEnv\(|AutomaticEnv\(|SetEnvPrefix\(' --include="*.go" . 2>/dev/null | head -50 +# Also inspect string literals that look like env-var keys +grep -rnE '"[A-Z][A-Z0-9_]*"' --include="*.go" . 2>/dev/null | head -100#!/bin/bash # Verify grep commands in this reference file that are missing a search root. # Expected: no lines where grep --include is used without trailing path (e.g., ".") rg -nP 'grep\s+-r[nlE]*\s+.*--include="[^"]+"\s+(?:2>/dev/null|\|)' \ hyperfleet-standards/skills/standards-audit/references/configuration-checks.mdAlso applies to: 69-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet-standards/skills/standards-audit/references/configuration-checks.md` around lines 42 - 43, The grep command in the reference uses no search root and only matches API calls (os.Getenv, Setenv, BindEnv, AutomaticEnv, SetEnvPrefix), which can miss files and doesn't validate actual env-var names; update the checks to run from a root (e.g., add "." after the --include clause) and replace or augment the pattern to search for literal env var usages (e.g., os.Getenv("PREFIX_), os.Setenv("PREFIX_), viper.BindEnv("PREFIX_"), or plain string constants matching the prefix) so the audit verifies actual env var names and scans all files; apply the same change to the other grep occurrences referenced in the file.
112-115:⚠️ Potential issue | 🟠 MajorConfig format detection is directory-scoped and can miss non-compliant files.
At Line 115, using
ls configs/ config/only checks two directories. Repos with config files elsewhere (or mixed formats) can incorrectly pass Check 11.Proposed fix
-**How to find:** `ls configs/ config/ 2>/dev/null` — check file extensions against the format required by the standard. +**How to find:** `find . -type f \( -iname "*.yaml" -o -iname "*.yml" -o -iname "*.json" -o -iname "*.toml" -o -iname "*.ini" \)` — compare discovered extensions with the format required by the standard and flag disallowed formats.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet-standards/skills/standards-audit/references/configuration-checks.md` around lines 112 - 115, The current Check 11 implementation only inspects the two directories via the `ls configs/ config/` command and thus misses config files elsewhere; update the check to scan the repository for config-like files (e.g., search for files with extensions .yaml/.yml/.json/.toml/.ini and common config filenames) rather than hardcoding `configs/` and `config/`, then flag any files whose extension or detected content type does not match the required YAML standard; update the description and the “How to find” guidance (referencing "Check 11: Config File Format" and the existing `ls configs/ config/` mention) to instruct using a repository-wide search for config files and content sniffing instead of directory-scoped listing.
84-85:⚠️ Potential issue | 🟠 MajorProperty naming check is still too narrow and truncates evidence.
At Line 85, the command only inspects two hardcoded files and truncates to 30 lines, so violations in other YAML files are easily missed.
Proposed fix
-**How to find:** `cat configs/config.yaml config/config.yaml 2>/dev/null | head -30` +**How to find:** `find . -type f \( -iname "*.yaml" -o -iname "*.yml" \) -print0 | xargs -0 -r grep -nE "^[[:space:]]*[A-Za-z0-9_.-]+:"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet-standards/skills/standards-audit/references/configuration-checks.md` around lines 84 - 85, The verification command `cat configs/config.yaml config/config.yaml 2>/dev/null | head -30` is too narrow and truncates results; replace it with a recursive search that covers all YAML files (both .yaml and .yml) and does not pipe to head so evidence isn't truncated — e.g., use a recursive grep/find that lists file paths and full matches (or prints full file contents) to ensure all config files are checked and no violations are missed; update the line in configuration-checks.md accordingly.
🧹 Nitpick comments (2)
hyperfleet-code-review/skills/review-pr/group-09-code-hygiene.md (1)
13-16: Pass 9b guidance currently under-specifies checks beyond log level.Line 13 includes framework selection and loop/hot-path constraints, but Line 15 only operationalizes level matching + intra-PR consistency. Please explicitly require checks for logging framework compliance and disallowed logging patterns in loops/hot paths to avoid enforcement drift.
Proposed wording update
-List every log statement added or modified in the diff. For each, evaluate whether the log level matches the requirements defined in the logging specification standard. Also flag inconsistent log levels for the same type of event within the PR. +List every log statement added or modified in the diff. For each, evaluate whether: +- the log level matches the logging specification standard, +- the logging library/framework usage matches the standard, and +- logging in loops/hot paths follows the standard’s constraints. +Also flag inconsistent log levels for the same type of event within the PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet-code-review/skills/review-pr/group-09-code-hygiene.md` around lines 13 - 16, Update the "Pass 9b" guidance to explicitly require (1) verification of the logging framework/library choice against the fetched logging specification standard and (2) checks for disallowed logging patterns in loops and hot paths (e.g., expensive formatting, synchronous I/O, or high-frequency debug logs). In the text that currently reads "Apply the log level requirements..." and the bullet that enforces "level matching + intra-PR consistency", add explicit sentences that require confirming the logging framework matches the standard and scanning every modified/added log site for loop/hot-path anti-patterns; also require emitting the mandatory finding "required logging specification standard unavailable" if the standard was not fetched. Reference this update under the "Pass 9b" section so reviewers enforce framework compliance and loop/hot-path constraints alongside log level checks.hyperfleet-code-review/skills/review-pr/SKILL.md (1)
220-220: Checklist wording is internally contradictory.“All 10 groups launched” conflicts with “1–7 and 10 skipped for non-Go diffs.” Rephrase to “all applicable groups launched” so checklist evaluation stays unambiguous.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet-code-review/skills/review-pr/SKILL.md` at line 220, The checklist item "[ ] All 10 mechanical pass groups launched in parallel (groups 1–7 and 10 skipped for non-Go diffs; groups 8–9 always run)" is self-contradictory; update the wording to remove ambiguity by replacing it with something like "All applicable mechanical pass groups launched in parallel (groups 8–9 always run; groups 1–7 and 10 are skipped for non‑Go diffs)" so the intent is clear and evaluation is unambiguous.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@hyperfleet-standards/skills/standards-audit/references/error-model-checks.md`:
- Around line 39-40: The grep invocations that search for error code patterns
(e.g., the command matching grep -rn "ErrorCode\|error_code\|ErrCode"
--include="*.go" 2>/dev/null and the other grep -rn commands in this document)
are missing an explicit search path and will read from stdin; update every grep
-rn invocation in this file to include a search path argument (use .)
immediately after the options so they become grep -rn "PATTERN"
--include="*.ext" . 2>/dev/null, ensuring all occurrences (the commands that
search for error codes and the similar grep lines later in the file) are
updated.
In
`@hyperfleet-standards/skills/standards-audit/references/generated-code-checks.md`:
- Line 15: Update the two recursive grep commands that omit the explicit search
path so they include the current-directory argument ("."); specifically modify
the occurrences of grep -rl "DO NOT EDIT\|Code
generated\|AUTO-GENERATED\|@generated" --include="*.go" 2>/dev/null (and the
similar command on line 21) to include . as the path argument to match the style
used elsewhere and ensure consistent behavior.
In
`@hyperfleet-standards/skills/standards-audit/references/health-endpoints-checks.md`:
- Around line 21-22: The grep commands that search for health/readiness patterns
(e.g., the line with grep -rn "health\|ready\|alive\|liveness\|readiness"
--include="*.go" and the other grep -rn invocations later in the file) are
missing an explicit search path, which can cause them to hang waiting for stdin;
update each of those grep -rn invocations to include an explicit path argument
(use .) after the include/options so they search the current directory (e.g.,
change grep -rn ... --include="*.go" to grep -rn ... --include="*.go" .) to
ensure commands do not block and behave reliably.
---
Outside diff comments:
In `@hyperfleet-code-review/skills/review-pr/SKILL.md`:
- Around line 109-117: Step 5 currently assumes standards from step 4b are
available; add a deterministic fallback so when the "standards fetch" (step 4b)
returns empty/error the Step 5 routine switches to a defined behavior: run only
intra-PR consistency checks, emit a mandatory "standards unavailable"
warning/note in the step output, and mark full standards-based validation as
skipped; ensure the Step 5 control flow explicitly tests for missing/erroneous
standards and returns this explicit status so downstream logic behaves
deterministically.
---
Duplicate comments:
In
`@hyperfleet-standards/skills/standards-audit/references/configuration-checks.md`:
- Around line 42-43: The grep command in the reference uses no search root and
only matches API calls (os.Getenv, Setenv, BindEnv, AutomaticEnv, SetEnvPrefix),
which can miss files and doesn't validate actual env-var names; update the
checks to run from a root (e.g., add "." after the --include clause) and replace
or augment the pattern to search for literal env var usages (e.g.,
os.Getenv("PREFIX_), os.Setenv("PREFIX_), viper.BindEnv("PREFIX_"), or plain
string constants matching the prefix) so the audit verifies actual env var names
and scans all files; apply the same change to the other grep occurrences
referenced in the file.
- Around line 112-115: The current Check 11 implementation only inspects the two
directories via the `ls configs/ config/` command and thus misses config files
elsewhere; update the check to scan the repository for config-like files (e.g.,
search for files with extensions .yaml/.yml/.json/.toml/.ini and common config
filenames) rather than hardcoding `configs/` and `config/`, then flag any files
whose extension or detected content type does not match the required YAML
standard; update the description and the “How to find” guidance (referencing
"Check 11: Config File Format" and the existing `ls configs/ config/` mention)
to instruct using a repository-wide search for config files and content sniffing
instead of directory-scoped listing.
- Around line 84-85: The verification command `cat configs/config.yaml
config/config.yaml 2>/dev/null | head -30` is too narrow and truncates results;
replace it with a recursive search that covers all YAML files (both .yaml and
.yml) and does not pipe to head so evidence isn't truncated — e.g., use a
recursive grep/find that lists file paths and full matches (or prints full file
contents) to ensure all config files are checked and no violations are missed;
update the line in configuration-checks.md accordingly.
In `@hyperfleet-standards/skills/standards-audit/references/helm-chart-checks.md`:
- Around line 124-125: The current grep -rn "broker\|kafka\|amqp\|nats\|mqtt"
charts/*/values.yaml check hardcodes protocol terms and only scans values.yaml,
so replace it with a YAML-aware discovery and validation: use recursive search
across chart directories (e.g., using grep -R --include='values.yaml' -E
'broker|kafka|amqp|nats|mqtt' OR better, iterate charts/* and parse values.yaml
with yq) to locate any broker config sections, then validate required fields
(TLS keys like tls.enabled/tls.ca/tls.cert, auth keys such as
auth.username/auth.password/auth.secretRef, and structured broker blocks) using
yq queries to ensure presence and types; update the doc to show the yq examples
and the grep pattern only as a fallback, and remove the hardcoded-only claim at
the current grep line.
---
Nitpick comments:
In `@hyperfleet-code-review/skills/review-pr/group-09-code-hygiene.md`:
- Around line 13-16: Update the "Pass 9b" guidance to explicitly require (1)
verification of the logging framework/library choice against the fetched logging
specification standard and (2) checks for disallowed logging patterns in loops
and hot paths (e.g., expensive formatting, synchronous I/O, or high-frequency
debug logs). In the text that currently reads "Apply the log level
requirements..." and the bullet that enforces "level matching + intra-PR
consistency", add explicit sentences that require confirming the logging
framework matches the standard and scanning every modified/added log site for
loop/hot-path anti-patterns; also require emitting the mandatory finding
"required logging specification standard unavailable" if the standard was not
fetched. Reference this update under the "Pass 9b" section so reviewers enforce
framework compliance and loop/hot-path constraints alongside log level checks.
In `@hyperfleet-code-review/skills/review-pr/SKILL.md`:
- Line 220: The checklist item "[ ] All 10 mechanical pass groups launched in
parallel (groups 1–7 and 10 skipped for non-Go diffs; groups 8–9 always run)" is
self-contradictory; update the wording to remove ambiguity by replacing it with
something like "All applicable mechanical pass groups launched in parallel
(groups 8–9 always run; groups 1–7 and 10 are skipped for non‑Go diffs)" so the
intent is clear and evaluation is unambiguous.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ca64922f-a0f3-4d6c-90c3-5d65d3bd064a
📒 Files selected for processing (28)
hyperfleet-code-review/.claude-plugin/plugin.jsonhyperfleet-code-review/skills/review-pr/SKILL.mdhyperfleet-code-review/skills/review-pr/group-01-error-handling.mdhyperfleet-code-review/skills/review-pr/group-02-concurrency.mdhyperfleet-code-review/skills/review-pr/group-03-exhaustiveness.mdhyperfleet-code-review/skills/review-pr/group-04-resource-lifecycle.mdhyperfleet-code-review/skills/review-pr/group-05-code-quality.mdhyperfleet-code-review/skills/review-pr/group-06-testing.mdhyperfleet-code-review/skills/review-pr/group-07-naming.mdhyperfleet-code-review/skills/review-pr/group-08-security.mdhyperfleet-code-review/skills/review-pr/group-09-code-hygiene.mdhyperfleet-code-review/skills/review-pr/group-10-performance.mdhyperfleet-code-review/skills/review-pr/mechanical-passes.mdhyperfleet-code-review/skills/review-pr/output-format.mdhyperfleet-standards/.claude-plugin/plugin.jsonhyperfleet-standards/skills/standards-audit/SKILL.mdhyperfleet-standards/skills/standards-audit/references/configuration-checks.mdhyperfleet-standards/skills/standards-audit/references/container-image-checks.mdhyperfleet-standards/skills/standards-audit/references/error-model-checks.mdhyperfleet-standards/skills/standards-audit/references/generated-code-checks.mdhyperfleet-standards/skills/standards-audit/references/graceful-shutdown-checks.mdhyperfleet-standards/skills/standards-audit/references/health-endpoints-checks.mdhyperfleet-standards/skills/standards-audit/references/helm-chart-checks.mdhyperfleet-standards/skills/standards-audit/references/linting-checks.mdhyperfleet-standards/skills/standards-audit/references/logging-checks.mdhyperfleet-standards/skills/standards-audit/references/makefile-checks.mdhyperfleet-standards/skills/standards-audit/references/metrics-checks.mdhyperfleet-standards/skills/standards-audit/references/tracing-checks.md
💤 Files with no reviewable changes (1)
- hyperfleet-code-review/skills/review-pr/mechanical-passes.md
✅ Files skipped from review due to trivial changes (11)
- hyperfleet-standards/.claude-plugin/plugin.json
- hyperfleet-code-review/.claude-plugin/plugin.json
- hyperfleet-code-review/skills/review-pr/group-04-resource-lifecycle.md
- hyperfleet-code-review/skills/review-pr/output-format.md
- hyperfleet-code-review/skills/review-pr/group-03-exhaustiveness.md
- hyperfleet-code-review/skills/review-pr/group-02-concurrency.md
- hyperfleet-code-review/skills/review-pr/group-07-naming.md
- hyperfleet-code-review/skills/review-pr/group-05-code-quality.md
- hyperfleet-standards/skills/standards-audit/references/metrics-checks.md
- hyperfleet-standards/skills/standards-audit/references/container-image-checks.md
- hyperfleet-code-review/skills/review-pr/group-06-testing.md
🚧 Files skipped from review as they are similar to previous changes (4)
- hyperfleet-standards/skills/standards-audit/references/makefile-checks.md
- hyperfleet-standards/skills/standards-audit/references/tracing-checks.md
- hyperfleet-standards/skills/standards-audit/references/logging-checks.md
- hyperfleet-standards/skills/standards-audit/references/graceful-shutdown-checks.md
hyperfleet-standards/skills/standards-audit/references/error-model-checks.md
Outdated
Show resolved
Hide resolved
hyperfleet-standards/skills/standards-audit/references/generated-code-checks.md
Outdated
Show resolved
Hide resolved
hyperfleet-standards/skills/standards-audit/references/health-endpoints-checks.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
hyperfleet-standards/skills/standards-audit/references/graceful-shutdown-checks.md (1)
92-93:⚠️ Potential issue | 🟠 MajorCheck 9 discovery is too broad to validate required shutdown test behaviors.
Line 92 expects behavioral coverage (signal handling, drain completion, timeout behavior), but Line 93 only keyword-matches test files. This can report PASS/PARTIAL without evidence of assertions for exit/timeout semantics.
Suggested doc fix
-**How to find:** `grep -rn "shutdown\|graceful\|signal\|SIGTERM" --include="*_test.go" 2>/dev/null` +**How to find:** +```bash +# Find shutdown-related test files and test cases +grep -rl "SIGTERM\|SIGINT\|Shutdown\|GracefulStop\|signal.Notify" --include="*_test.go" 2>/dev/null +grep -rn "TestShutdown\|TestGraceful\|TestSignal\|TestDrain\|TestTimeout" --include="*_test.go" 2>/dev/null + +# Verify assertions for clean exit / timeout semantics +grep -rn "os.Exit\|ExitCode\|wait.*status\|exec.ExitError\|context.DeadlineExceeded" --include="*_test.go" 2>/dev/null +```As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet-standards/skills/standards-audit/references/graceful-shutdown-checks.md` around lines 92 - 93, Update the discovery step to look for specific shutdown-related test names and assertions rather than only keyword matches: search test files for signal handling and shutdown helpers (e.g., "SIGTERM", "SIGINT", "signal.Notify", "Shutdown", "GracefulStop") and for explicit test names ("TestShutdown", "TestGraceful", "TestSignal", "TestDrain", "TestTimeout"), then verify presence of assertions/handling for exit and timeout semantics by checking for patterns like "os.Exit", "ExitCode", "context.DeadlineExceeded", "exec.ExitError", or wait/status checks; apply these checks to the code examples and replace the broad grep on line 93 with these targeted grep patterns so the standard validates behavioral assertions, not just keyword occurrence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hyperfleet-code-review/skills/review-pr/SKILL.md`:
- Around line 109-110: The standards-unavailable fallback is inconsistent: when
"standards fetch (step 4b)" fails the doc still instructs running "intra-PR
consistency checks (bullets 1–3)" even though those bullets contain
standards-dependent checks, and the checklist still mandates standards-based
consistency. Update the flow so that when standards are unavailable the text
explicitly removes or disables any standards-dependent items from "bullets 1–3"
(identify and strip checks that reference standards, compliance, or standard
IDs), add a clear mandatory note "HyperFleet standards unavailable — skipping
standards-based validation", and change the checklist entries that require
standards-based consistency to show those checks as skipped/NA; also adjust
"Step 5" bullets to no longer reference or require standards-based verification
in this code path.
- Around line 43-44: The documentation currently uses an undefined "repository
project key pattern" when checking PR titles like "PROJ-123"; update the
workflow to explicitly describe how to discover that pattern (e.g., read a
configured project key from repository metadata or an env/config variable, fall
back to deriving the key from the repo name or from a CODEOWNERS/manifest entry)
and then validate the PR title against that discovered pattern before running
the jira CLI command (`jira issue view <TICKET-ID> --comments 50`). Modify
SKILL.md to list the exact lookup order (primary: repo config/env var,
secondary: repo name heuristic, tertiary: skip/flag) and specify that the
discovered key is used to construct the regex for ticket validation.
- Around line 63-66: Change the standards-fetch step so it retrieves all
standards files instead of pre-filtering by “relevant”; specifically, remove the
relevance pre-filter around the hyperfleet-architecture invocation and call
"Fetch the full content of hyperfleet/standards/*.md" (or iterate all filenames)
once (in parallel) to collect the complete set of standards, then pass that
complete collection into the mechanical checks (step 4e/group files) and the
intra-PR consistency check (step 5) so they can consume whichever documents they
require.
In
`@hyperfleet-standards/skills/standards-audit/references/error-model-checks.md`:
- Around line 39-40: The grep invocation hardcodes the token set
"ErrorCode|error_code|ErrCode", which conflicts with the standard-driven
error-code prefix; update the check to read the canonical token/prefix list from
the standard configuration (or a referenced variable) instead of hardcoding
these tokens—e.g., replace the literal pattern in the grep call with a variable
like ${ERROR_CODE_PATTERNS} (or a helper that loads the prefix list from the
standards file) so the search for error codes always uses the current
standard-driven tokens rather than "ErrorCode|error_code|ErrCode".
In
`@hyperfleet-standards/skills/standards-audit/references/graceful-shutdown-checks.md`:
- Around line 39-40: The grep command in graceful-shutdown-checks.md hardcodes
timeout key patterns ("shutdownTimeout\|gracePeriod\|terminationGrace\|Timeout")
which can drift from the formal standard; update the guidance to reference the
canonical list of timeout identifiers from the standard rather than hardcoding
values—e.g., instruct readers to build the search pattern from the standard’s
identifier list (placeholder like STANDARD_TIMEOUT_KEYS) or to run a command
that reads the standard file and supplies the keys to grep, and replace the
literal pattern in the example with that reference so the document and the
pattern remain in sync.
---
Duplicate comments:
In
`@hyperfleet-standards/skills/standards-audit/references/graceful-shutdown-checks.md`:
- Around line 92-93: Update the discovery step to look for specific
shutdown-related test names and assertions rather than only keyword matches:
search test files for signal handling and shutdown helpers (e.g., "SIGTERM",
"SIGINT", "signal.Notify", "Shutdown", "GracefulStop") and for explicit test
names ("TestShutdown", "TestGraceful", "TestSignal", "TestDrain",
"TestTimeout"), then verify presence of assertions/handling for exit and timeout
semantics by checking for patterns like "os.Exit", "ExitCode",
"context.DeadlineExceeded", "exec.ExitError", or wait/status checks; apply these
checks to the code examples and replace the broad grep on line 93 with these
targeted grep patterns so the standard validates behavioral assertions, not just
keyword occurrence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 049d3f5f-39bb-4d7b-86e5-be83ec4cd391
📒 Files selected for processing (28)
hyperfleet-code-review/.claude-plugin/plugin.jsonhyperfleet-code-review/skills/review-pr/SKILL.mdhyperfleet-code-review/skills/review-pr/group-01-error-handling.mdhyperfleet-code-review/skills/review-pr/group-02-concurrency.mdhyperfleet-code-review/skills/review-pr/group-03-exhaustiveness.mdhyperfleet-code-review/skills/review-pr/group-04-resource-lifecycle.mdhyperfleet-code-review/skills/review-pr/group-05-code-quality.mdhyperfleet-code-review/skills/review-pr/group-06-testing.mdhyperfleet-code-review/skills/review-pr/group-07-naming.mdhyperfleet-code-review/skills/review-pr/group-08-security.mdhyperfleet-code-review/skills/review-pr/group-09-code-hygiene.mdhyperfleet-code-review/skills/review-pr/group-10-performance.mdhyperfleet-code-review/skills/review-pr/mechanical-passes.mdhyperfleet-code-review/skills/review-pr/output-format.mdhyperfleet-standards/.claude-plugin/plugin.jsonhyperfleet-standards/skills/standards-audit/SKILL.mdhyperfleet-standards/skills/standards-audit/references/configuration-checks.mdhyperfleet-standards/skills/standards-audit/references/container-image-checks.mdhyperfleet-standards/skills/standards-audit/references/error-model-checks.mdhyperfleet-standards/skills/standards-audit/references/generated-code-checks.mdhyperfleet-standards/skills/standards-audit/references/graceful-shutdown-checks.mdhyperfleet-standards/skills/standards-audit/references/health-endpoints-checks.mdhyperfleet-standards/skills/standards-audit/references/helm-chart-checks.mdhyperfleet-standards/skills/standards-audit/references/linting-checks.mdhyperfleet-standards/skills/standards-audit/references/logging-checks.mdhyperfleet-standards/skills/standards-audit/references/makefile-checks.mdhyperfleet-standards/skills/standards-audit/references/metrics-checks.mdhyperfleet-standards/skills/standards-audit/references/tracing-checks.md
💤 Files with no reviewable changes (1)
- hyperfleet-code-review/skills/review-pr/mechanical-passes.md
✅ Files skipped from review due to trivial changes (10)
- hyperfleet-standards/.claude-plugin/plugin.json
- hyperfleet-code-review/.claude-plugin/plugin.json
- hyperfleet-code-review/skills/review-pr/output-format.md
- hyperfleet-code-review/skills/review-pr/group-05-code-quality.md
- hyperfleet-standards/skills/standards-audit/references/makefile-checks.md
- hyperfleet-code-review/skills/review-pr/group-07-naming.md
- hyperfleet-code-review/skills/review-pr/group-02-concurrency.md
- hyperfleet-code-review/skills/review-pr/group-08-security.md
- hyperfleet-standards/skills/standards-audit/references/container-image-checks.md
- hyperfleet-code-review/skills/review-pr/group-03-exhaustiveness.md
🚧 Files skipped from review as they are similar to previous changes (7)
- hyperfleet-standards/skills/standards-audit/references/tracing-checks.md
- hyperfleet-standards/skills/standards-audit/references/helm-chart-checks.md
- hyperfleet-code-review/skills/review-pr/group-06-testing.md
- hyperfleet-code-review/skills/review-pr/group-04-resource-lifecycle.md
- hyperfleet-standards/skills/standards-audit/references/configuration-checks.md
- hyperfleet-standards/skills/standards-audit/references/health-endpoints-checks.md
- hyperfleet-standards/skills/standards-audit/references/metrics-checks.md
hyperfleet-standards/skills/standards-audit/references/error-model-checks.md
Show resolved
Hide resolved
hyperfleet-standards/skills/standards-audit/references/graceful-shutdown-checks.md
Show resolved
Hide resolved
There was a problem hiding this comment.
♻️ Duplicate comments (3)
hyperfleet-standards/skills/standards-audit/references/error-model-checks.md (1)
39-40:⚠️ Potential issue | 🟠 MajorHardcoded error-code patterns contradict the standard-driven approach.
Line 39 instructs to "search for the error code prefix defined in the standard," but line 40 hardcodes
ErrorCode|error_code|ErrCode. This reintroduces drift—the same issue the PR aims to eliminate. If the standard uses a different naming scheme, this grep will miss valid implementations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet-standards/skills/standards-audit/references/error-model-checks.md` around lines 39 - 40, The grep step hardcodes error-name patterns (ErrorCode, error_code, ErrCode) which conflicts with the standard-driven lookup; update the check to derive the prefix/pattern from the standard configuration or a single canonical variable and use that value in the search instead of literal patterns (replace the hardcoded "ErrorCode|error_code|ErrCode" with a dynamic pattern sourced from the standard, e.g., use the configured error prefix or a generated regex based on the standard's name to search for occurrences of the error identifier).hyperfleet-code-review/skills/review-pr/SKILL.md (2)
223-223:⚠️ Potential issue | 🟠 MajorChecklist contradicts the standards-unavailable fallback path.
Line 223 always requires standards-based consistency, but Line 109-Line 110 explicitly allows skipping standards-dependent checks when standards fetch fails. This creates conflicting execution criteria.
Suggested doc fix
-- [ ] Intra-PR consistency checked against HyperFleet standards +- [ ] Intra-PR consistency checked against HyperFleet standards (or marked skipped when standards are unavailable)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet-code-review/skills/review-pr/SKILL.md` at line 223, The checklist item "Intra-PR consistency checked against HyperFleet standards" conflicts with the standards-unavailable fallback; update that checkbox text in SKILL.md to reflect conditional execution (e.g., "If HyperFleet standards are available, check intra-PR consistency against them (skip if standards fetch fails)") or add a parenthetical note linking it to the existing "standards-unavailable fallback" behavior so standards-dependent checks are explicit and consistent with the earlier allowance to skip when standards cannot be fetched.
43-44:⚠️ Potential issue | 🟠 MajorDefine repository project-key discovery before ticket validation.
Line 43 uses a “project key pattern from the repository” but the workflow never defines how to derive it. That makes JIRA checks non-deterministic and can silently miss valid tickets or accept invalid ones.
Suggested doc fix
-- If there is a JIRA ticket in the PR title (matching the project key pattern from the repository, e.g. `PROJ-123`): +- Derive the repository JIRA project key pattern first (for example: repo config/env standard → explicit repo metadata → fallback heuristic). Then, if the PR title contains a ticket matching that derived pattern (e.g. `PROJ-123`):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet-code-review/skills/review-pr/SKILL.md` around lines 43 - 44, Replace the vague "project key pattern from the repository" with a deterministic discovery procedure: document and implement extracting the JIRA project key by first checking an explicit repository config field (e.g., a declared jira_project_key in repo metadata), then falling back to parsing the git remote URL or repository name with a strict regex, validating the extracted key before using it to detect tickets in the PR title, and use that validated key when invoking the jira CLI command (jira issue view <TICKET-ID> --comments 50); update the SKILL.md text to describe this exact precedence and validation so ticket checks are deterministic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@hyperfleet-code-review/skills/review-pr/SKILL.md`:
- Line 223: The checklist item "Intra-PR consistency checked against HyperFleet
standards" conflicts with the standards-unavailable fallback; update that
checkbox text in SKILL.md to reflect conditional execution (e.g., "If HyperFleet
standards are available, check intra-PR consistency against them (skip if
standards fetch fails)") or add a parenthetical note linking it to the existing
"standards-unavailable fallback" behavior so standards-dependent checks are
explicit and consistent with the earlier allowance to skip when standards cannot
be fetched.
- Around line 43-44: Replace the vague "project key pattern from the repository"
with a deterministic discovery procedure: document and implement extracting the
JIRA project key by first checking an explicit repository config field (e.g., a
declared jira_project_key in repo metadata), then falling back to parsing the
git remote URL or repository name with a strict regex, validating the extracted
key before using it to detect tickets in the PR title, and use that validated
key when invoking the jira CLI command (jira issue view <TICKET-ID> --comments
50); update the SKILL.md text to describe this exact precedence and validation
so ticket checks are deterministic.
In
`@hyperfleet-standards/skills/standards-audit/references/error-model-checks.md`:
- Around line 39-40: The grep step hardcodes error-name patterns (ErrorCode,
error_code, ErrCode) which conflicts with the standard-driven lookup; update the
check to derive the prefix/pattern from the standard configuration or a single
canonical variable and use that value in the search instead of literal patterns
(replace the hardcoded "ErrorCode|error_code|ErrCode" with a dynamic pattern
sourced from the standard, e.g., use the configured error prefix or a generated
regex based on the standard's name to search for occurrences of the error
identifier).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 43c5a200-1638-4db2-a1ef-f09470efe5af
📒 Files selected for processing (28)
hyperfleet-code-review/.claude-plugin/plugin.jsonhyperfleet-code-review/skills/review-pr/SKILL.mdhyperfleet-code-review/skills/review-pr/group-01-error-handling.mdhyperfleet-code-review/skills/review-pr/group-02-concurrency.mdhyperfleet-code-review/skills/review-pr/group-03-exhaustiveness.mdhyperfleet-code-review/skills/review-pr/group-04-resource-lifecycle.mdhyperfleet-code-review/skills/review-pr/group-05-code-quality.mdhyperfleet-code-review/skills/review-pr/group-06-testing.mdhyperfleet-code-review/skills/review-pr/group-07-naming.mdhyperfleet-code-review/skills/review-pr/group-08-security.mdhyperfleet-code-review/skills/review-pr/group-09-code-hygiene.mdhyperfleet-code-review/skills/review-pr/group-10-performance.mdhyperfleet-code-review/skills/review-pr/mechanical-passes.mdhyperfleet-code-review/skills/review-pr/output-format.mdhyperfleet-standards/.claude-plugin/plugin.jsonhyperfleet-standards/skills/standards-audit/SKILL.mdhyperfleet-standards/skills/standards-audit/references/configuration-checks.mdhyperfleet-standards/skills/standards-audit/references/container-image-checks.mdhyperfleet-standards/skills/standards-audit/references/error-model-checks.mdhyperfleet-standards/skills/standards-audit/references/generated-code-checks.mdhyperfleet-standards/skills/standards-audit/references/graceful-shutdown-checks.mdhyperfleet-standards/skills/standards-audit/references/health-endpoints-checks.mdhyperfleet-standards/skills/standards-audit/references/helm-chart-checks.mdhyperfleet-standards/skills/standards-audit/references/linting-checks.mdhyperfleet-standards/skills/standards-audit/references/logging-checks.mdhyperfleet-standards/skills/standards-audit/references/makefile-checks.mdhyperfleet-standards/skills/standards-audit/references/metrics-checks.mdhyperfleet-standards/skills/standards-audit/references/tracing-checks.md
💤 Files with no reviewable changes (1)
- hyperfleet-code-review/skills/review-pr/mechanical-passes.md
✅ Files skipped from review due to trivial changes (11)
- hyperfleet-standards/.claude-plugin/plugin.json
- hyperfleet-code-review/.claude-plugin/plugin.json
- hyperfleet-code-review/skills/review-pr/group-05-code-quality.md
- hyperfleet-code-review/skills/review-pr/group-03-exhaustiveness.md
- hyperfleet-code-review/skills/review-pr/group-07-naming.md
- hyperfleet-standards/skills/standards-audit/references/container-image-checks.md
- hyperfleet-standards/skills/standards-audit/references/makefile-checks.md
- hyperfleet-code-review/skills/review-pr/group-06-testing.md
- hyperfleet-code-review/skills/review-pr/group-02-concurrency.md
- hyperfleet-code-review/skills/review-pr/group-08-security.md
- hyperfleet-code-review/skills/review-pr/group-04-resource-lifecycle.md
🚧 Files skipped from review as they are similar to previous changes (7)
- hyperfleet-code-review/skills/review-pr/output-format.md
- hyperfleet-standards/skills/standards-audit/references/metrics-checks.md
- hyperfleet-standards/skills/standards-audit/references/linting-checks.md
- hyperfleet-standards/skills/standards-audit/references/configuration-checks.md
- hyperfleet-standards/skills/standards-audit/references/graceful-shutdown-checks.md
- hyperfleet-standards/skills/standards-audit/references/health-endpoints-checks.md
- hyperfleet-standards/skills/standards-audit/references/helm-chart-checks.md
pnguyen44
reviewed
Apr 6, 2026
pnguyen44
left a comment
pnguyen44
left a comment
There was a problem hiding this comment.
The group files mix rule definitions with checking mechanics. If these are going to be the team's standards enforced by PR review and CI, consider extracting them into the architecture repo under hyperfleet/standards/ alongside linting.md, then having each group reference its standard by name. This gives us one source of truth with no duplication, and makes the rules reusable by any skill or tool.
Contributor
Author
|
Good point — I considered this split but went with the current design intentionally. Here's the reasoning: The group files have two distinct categories of passes:
Moving the generic Go passes into the architecture repo would mean:
The current design draws the line at: if it's a HyperFleet-specific decision (error model, naming, logging spec), it lives in the architecture repo as the source of truth. If it's a universally accepted Go practice (check your errors, don't capture loop variables, don't defer in loops), it's defined inline in the review mechanic. That said, if the team wants to formally codify these Go practices as team standards (e.g., to set specific thresholds like "max function length = 50 lines"), then extracting them would make sense. Happy to revisit if the team leans that way. |
|
Good point on the fetch dependency concern. Worth noting though, step 4b already fetches all standards from hyperfleet/standards/ in parallel in a single pass, so adding more docs there would be absorbed into the existing concurrent fetch with negligible cost difference. On the governance overhead point, if these checks are going into CI and blocking merges, the governance decision has already been made. Documenting them in the architecture repo just makes that explicit rather than leaving it buried in skill mechanics files. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
hyperfleet-code-review/skills/review-pr/SKILL.md (1)
232-232:⚠️ Potential issue | 🟠 MajorChecklist should reflect the standards-unavailable skip path.
Line 232 is unconditional, but Step 5 explicitly allows standards-based checks to be skipped when standards fetch fails. Mark this checklist item as “or skipped/NA when standards are unavailable” to keep execution and checklist consistent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet-code-review/skills/review-pr/SKILL.md` at line 232, Update the checklist item "- [ ] Intra-PR consistency checked against HyperFleet standards" to indicate it can be skipped/NA when standards are unavailable (per Step 5); e.g., change the label to something like "Intra-PR consistency checked against HyperFleet standards (or mark as SKIPPED/NA if standards unavailable)" so the checklist text matches the allowed skip path described in Step 5 and keeps execution/checklist consistent.hyperfleet-standards/skills/standards-audit/references/configuration-checks.md (1)
114-115:⚠️ Potential issue | 🟠 MajorCheck 11 discovery is too narrow and can false-pass format compliance
This only inspects top-level
configs/andconfig/entries, so JSON/TOML/INI files in nested paths or alternate config locations can be missed.Proposed fix
-**How to find:** `ls configs/ config/ 2>/dev/null` — check file extensions against the format required by the standard. +**How to find:** `find . \( -iname "*.yaml" -o -iname "*.yml" -o -iname "*.json" -o -iname "*.toml" -o -iname "*.ini" \) 2>/dev/null` — verify discovered config file extensions against the format required by the standard.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet-standards/skills/standards-audit/references/configuration-checks.md` around lines 114 - 115, The current "How to find" step for Check 11 only lists top-level directories (`ls configs/ config/`) which can miss nested or alternate locations; update the guidance for Check 11 to perform a recursive search (e.g., using find or recursive directory traversal) across the repository and common config locations to match file extensions against the mandated format (YAML) and flag files with JSON/TOML/INI extensions, and mention checking typical alternate paths (e.g., */config/*, */configs/*, env-specific config folders) so nested or non-top-level config files are not false-passed.hyperfleet-standards/skills/standards-audit/references/generated-code-checks.md (1)
7-7:⚠️ Potential issue | 🟠 MajorReference doc fetch source is inconsistent with the orchestrator contract
This says standards come from the
hyperfleet-architectureskill, buthyperfleet-standards/skills/standards-audit/SKILL.md(Lines 144-167) defines orchestrator-supplied content fetched viagh api. Keep this wording aligned to avoid agent drift.Proposed fix
-Use the standard document content provided by the orchestrator (fetched via the `hyperfleet-architecture` skill). The orchestrator passes the full standard content to each agent — no additional fetching is needed. +Use the standard document content provided by the orchestrator (fetched via `gh api`). The orchestrator passes the full standard content to each agent — no additional fetching is needed.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet-standards/skills/standards-audit/references/generated-code-checks.md` at line 7, Update the inconsistent wording so both documents agree that the orchestrator supplies the full standard content: modify generated-code-checks.md to remove the claim that agents must fetch the standard via the hyperfleet-architecture skill and instead state that the orchestrator passes the full standard content (as described in SKILL.md), or alternatively update hyperfleet-standards/skills/standards-audit/SKILL.md to explicitly state that the orchestrator provides the gh api-fetched content to agents; ensure references to "hyperfleet-architecture" and the orchestrator contract are consistent between generated-code-checks.md and SKILL.md so agents do not attempt additional fetching.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hyperfleet-code-review/skills/review-pr/group-01-error-handling.md`:
- Around line 19-21: Replace the informal/variant references to the error
standard with the canonical name "Error Model" wherever the diff compares
against or mentions the standard (specifically the comparison sentence and the
mandatory finding message), ensuring the comparison step reads "Compare against
the Error Model" and the emitted finding uses the exact phrase "Error Model" so
lookups won't be treated as unavailable; update any nearby phrasing that
currently says variants like "error model standard" to the exact canonical name
to avoid ambiguity.
In `@hyperfleet-code-review/skills/review-pr/group-09-code-hygiene.md`:
- Around line 7-13: Rename the standard names used in this file to the exact
canonical phrases: replace any variant of "commit message standard" with "Commit
Message Standard" and any variant of "logging specification" with "Logging
Specification" (affecting the headings/mentions in the Pass 9a/9b sections such
as the lines that currently read "Apply the commit message and code comment
requirements..." and "Apply the log level requirements..."); ensure the two
exact strings "Commit Message Standard" and "Logging Specification" appear where
the standards are referenced so resolution logic finds them.
In `@hyperfleet-code-review/skills/review-pr/SKILL.md`:
- Around line 58-75: Step 4b currently fetches standards directly with gh CLI;
instead modify the "Fetch HyperFleet standards" step to call the
hyperfleet-architecture skill to retrieve the standards (and return their
content) and pass that content into the mechanical checks (step 4e) and intra-PR
consistency check (step 5); remove or guard the direct gh API loop, add a clear
fallback that notes in the summary if the hyperfleet-architecture skill is
unavailable, and ensure the produced output format matches what the downstream
checks expect.
In `@hyperfleet-standards/skills/standards-audit/SKILL.md`:
- Around line 45-53: The fetch loop over STANDARDS_FILES does not record which
filenames failed, so the final audit cannot report which standards failed to
fetch; modify the loop that iterates "for FILE in $STANDARDS_FILES" to detect
non-zero exit from the gh api call and append failing FILE names to a persistent
collection (e.g., an array or comma-separated string) while suppressing per-file
output so only final results are shown, then after the loop output the audit
results and, if the failure collection is non-empty, include a clear summary of
which FILEs failed to fetch; ensure the logic around the gh api invocation and
the failure accumulator are in the same scope as the loop so failures persist.
---
Duplicate comments:
In `@hyperfleet-code-review/skills/review-pr/SKILL.md`:
- Line 232: Update the checklist item "- [ ] Intra-PR consistency checked
against HyperFleet standards" to indicate it can be skipped/NA when standards
are unavailable (per Step 5); e.g., change the label to something like "Intra-PR
consistency checked against HyperFleet standards (or mark as SKIPPED/NA if
standards unavailable)" so the checklist text matches the allowed skip path
described in Step 5 and keeps execution/checklist consistent.
In
`@hyperfleet-standards/skills/standards-audit/references/configuration-checks.md`:
- Around line 114-115: The current "How to find" step for Check 11 only lists
top-level directories (`ls configs/ config/`) which can miss nested or alternate
locations; update the guidance for Check 11 to perform a recursive search (e.g.,
using find or recursive directory traversal) across the repository and common
config locations to match file extensions against the mandated format (YAML) and
flag files with JSON/TOML/INI extensions, and mention checking typical alternate
paths (e.g., */config/*, */configs/*, env-specific config folders) so nested or
non-top-level config files are not false-passed.
In
`@hyperfleet-standards/skills/standards-audit/references/generated-code-checks.md`:
- Line 7: Update the inconsistent wording so both documents agree that the
orchestrator supplies the full standard content: modify generated-code-checks.md
to remove the claim that agents must fetch the standard via the
hyperfleet-architecture skill and instead state that the orchestrator passes the
full standard content (as described in SKILL.md), or alternatively update
hyperfleet-standards/skills/standards-audit/SKILL.md to explicitly state that
the orchestrator provides the gh api-fetched content to agents; ensure
references to "hyperfleet-architecture" and the orchestrator contract are
consistent between generated-code-checks.md and SKILL.md so agents do not
attempt additional fetching.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a33cafea-db99-462f-b904-080e81aa6644
📒 Files selected for processing (28)
hyperfleet-code-review/.claude-plugin/plugin.jsonhyperfleet-code-review/skills/review-pr/SKILL.mdhyperfleet-code-review/skills/review-pr/group-01-error-handling.mdhyperfleet-code-review/skills/review-pr/group-02-concurrency.mdhyperfleet-code-review/skills/review-pr/group-03-exhaustiveness.mdhyperfleet-code-review/skills/review-pr/group-04-resource-lifecycle.mdhyperfleet-code-review/skills/review-pr/group-05-code-quality.mdhyperfleet-code-review/skills/review-pr/group-06-testing.mdhyperfleet-code-review/skills/review-pr/group-07-naming.mdhyperfleet-code-review/skills/review-pr/group-08-security.mdhyperfleet-code-review/skills/review-pr/group-09-code-hygiene.mdhyperfleet-code-review/skills/review-pr/group-10-performance.mdhyperfleet-code-review/skills/review-pr/mechanical-passes.mdhyperfleet-code-review/skills/review-pr/output-format.mdhyperfleet-standards/.claude-plugin/plugin.jsonhyperfleet-standards/skills/standards-audit/SKILL.mdhyperfleet-standards/skills/standards-audit/references/configuration-checks.mdhyperfleet-standards/skills/standards-audit/references/container-image-checks.mdhyperfleet-standards/skills/standards-audit/references/error-model-checks.mdhyperfleet-standards/skills/standards-audit/references/generated-code-checks.mdhyperfleet-standards/skills/standards-audit/references/graceful-shutdown-checks.mdhyperfleet-standards/skills/standards-audit/references/health-endpoints-checks.mdhyperfleet-standards/skills/standards-audit/references/helm-chart-checks.mdhyperfleet-standards/skills/standards-audit/references/linting-checks.mdhyperfleet-standards/skills/standards-audit/references/logging-checks.mdhyperfleet-standards/skills/standards-audit/references/makefile-checks.mdhyperfleet-standards/skills/standards-audit/references/metrics-checks.mdhyperfleet-standards/skills/standards-audit/references/tracing-checks.md
💤 Files with no reviewable changes (1)
- hyperfleet-code-review/skills/review-pr/mechanical-passes.md
✅ Files skipped from review due to trivial changes (8)
- hyperfleet-standards/.claude-plugin/plugin.json
- hyperfleet-code-review/.claude-plugin/plugin.json
- hyperfleet-code-review/skills/review-pr/group-03-exhaustiveness.md
- hyperfleet-code-review/skills/review-pr/group-05-code-quality.md
- hyperfleet-code-review/skills/review-pr/group-07-naming.md
- hyperfleet-code-review/skills/review-pr/group-06-testing.md
- hyperfleet-code-review/skills/review-pr/group-08-security.md
- hyperfleet-code-review/skills/review-pr/group-04-resource-lifecycle.md
🚧 Files skipped from review as they are similar to previous changes (11)
- hyperfleet-code-review/skills/review-pr/output-format.md
- hyperfleet-standards/skills/standards-audit/references/makefile-checks.md
- hyperfleet-standards/skills/standards-audit/references/tracing-checks.md
- hyperfleet-standards/skills/standards-audit/references/linting-checks.md
- hyperfleet-standards/skills/standards-audit/references/metrics-checks.md
- hyperfleet-standards/skills/standards-audit/references/graceful-shutdown-checks.md
- hyperfleet-standards/skills/standards-audit/references/health-endpoints-checks.md
- hyperfleet-standards/skills/standards-audit/references/container-image-checks.md
- hyperfleet-standards/skills/standards-audit/references/logging-checks.md
- hyperfleet-code-review/skills/review-pr/group-02-concurrency.md
- hyperfleet-standards/skills/standards-audit/references/helm-chart-checks.md
hyperfleet-code-review/skills/review-pr/group-01-error-handling.md
Outdated
Show resolved
Hide resolved
hyperfleet-code-review/skills/review-pr/group-09-code-hygiene.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hyperfleet-code-review/skills/review-pr/SKILL.md`:
- Line 227: Update the checklist item that currently reads "HyperFleet standards
fetched via `hyperfleet-architecture` skill" to reflect the actual
implementation in Step 4b which uses the gh CLI; specifically, edit the
checklist entry in SKILL.md to either say "HyperFleet standards fetched via gh
CLI (or via `hyperfleet-architecture` skill if available)" or mark the skill as
optional/skipped, ensuring the wording matches Step 4b's behavior so reviewers
are not misled; reference the checklist line text and Step 4b when making the
change.
In `@hyperfleet-standards/skills/standards-audit/SKILL.md`:
- Line 152: Reference docs disagree: SKILL.md (line 152) and implementation
state standards are "fetched by the orchestrator via `gh api`" while all
reference files' Step 1 still say "fetched via the `hyperfleet-architecture`
skill"; update each reference file's Step 1 (configuration-checks.md,
container-image-checks.md, dependency-pinning-checks.md,
directory-structure-checks.md, error-model-checks.md, generated-code-checks.md,
graceful-shutdown-checks.md, health-endpoints-checks.md, helm-chart-checks.md,
linting-checks.md, logging-checks.md, makefile-checks.md, metrics-checks.md,
tracing-checks.md) to replace the phrase "fetched via the
`hyperfleet-architecture` skill" with "fetched by the orchestrator via `gh api`"
so the docs match the actual `gh api` implementation called in SKILL.md.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fe25c23c-f192-4a92-8ed6-8b60955509e2
📒 Files selected for processing (28)
hyperfleet-code-review/.claude-plugin/plugin.jsonhyperfleet-code-review/skills/review-pr/SKILL.mdhyperfleet-code-review/skills/review-pr/group-01-error-handling.mdhyperfleet-code-review/skills/review-pr/group-02-concurrency.mdhyperfleet-code-review/skills/review-pr/group-03-exhaustiveness.mdhyperfleet-code-review/skills/review-pr/group-04-resource-lifecycle.mdhyperfleet-code-review/skills/review-pr/group-05-code-quality.mdhyperfleet-code-review/skills/review-pr/group-06-testing.mdhyperfleet-code-review/skills/review-pr/group-07-naming.mdhyperfleet-code-review/skills/review-pr/group-08-security.mdhyperfleet-code-review/skills/review-pr/group-09-code-hygiene.mdhyperfleet-code-review/skills/review-pr/group-10-performance.mdhyperfleet-code-review/skills/review-pr/mechanical-passes.mdhyperfleet-code-review/skills/review-pr/output-format.mdhyperfleet-standards/.claude-plugin/plugin.jsonhyperfleet-standards/skills/standards-audit/SKILL.mdhyperfleet-standards/skills/standards-audit/references/configuration-checks.mdhyperfleet-standards/skills/standards-audit/references/container-image-checks.mdhyperfleet-standards/skills/standards-audit/references/error-model-checks.mdhyperfleet-standards/skills/standards-audit/references/generated-code-checks.mdhyperfleet-standards/skills/standards-audit/references/graceful-shutdown-checks.mdhyperfleet-standards/skills/standards-audit/references/health-endpoints-checks.mdhyperfleet-standards/skills/standards-audit/references/helm-chart-checks.mdhyperfleet-standards/skills/standards-audit/references/linting-checks.mdhyperfleet-standards/skills/standards-audit/references/logging-checks.mdhyperfleet-standards/skills/standards-audit/references/makefile-checks.mdhyperfleet-standards/skills/standards-audit/references/metrics-checks.mdhyperfleet-standards/skills/standards-audit/references/tracing-checks.md
💤 Files with no reviewable changes (1)
- hyperfleet-code-review/skills/review-pr/mechanical-passes.md
✅ Files skipped from review due to trivial changes (9)
- hyperfleet-standards/.claude-plugin/plugin.json
- hyperfleet-code-review/.claude-plugin/plugin.json
- hyperfleet-code-review/skills/review-pr/group-05-code-quality.md
- hyperfleet-code-review/skills/review-pr/group-08-security.md
- hyperfleet-code-review/skills/review-pr/group-06-testing.md
- hyperfleet-code-review/skills/review-pr/group-04-resource-lifecycle.md
- hyperfleet-code-review/skills/review-pr/group-02-concurrency.md
- hyperfleet-code-review/skills/review-pr/group-07-naming.md
- hyperfleet-code-review/skills/review-pr/group-01-error-handling.md
🚧 Files skipped from review as they are similar to previous changes (11)
- hyperfleet-code-review/skills/review-pr/output-format.md
- hyperfleet-standards/skills/standards-audit/references/tracing-checks.md
- hyperfleet-standards/skills/standards-audit/references/metrics-checks.md
- hyperfleet-code-review/skills/review-pr/group-03-exhaustiveness.md
- hyperfleet-standards/skills/standards-audit/references/linting-checks.md
- hyperfleet-standards/skills/standards-audit/references/graceful-shutdown-checks.md
- hyperfleet-standards/skills/standards-audit/references/configuration-checks.md
- hyperfleet-standards/skills/standards-audit/references/health-endpoints-checks.md
- hyperfleet-standards/skills/standards-audit/references/logging-checks.md
- hyperfleet-standards/skills/standards-audit/references/makefile-checks.md
- hyperfleet-standards/skills/standards-audit/references/error-model-checks.md
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
hyperfleet-standards/skills/standards-audit/references/configuration-checks.md (1)
112-116:⚠️ Potential issue | 🟠 MajorConfig format check can miss non-YAML files outside
config*/directories.Line 115 scopes discovery to
configs/andconfig/only, so repos with active config files elsewhere can incorrectly pass Check 11.Suggested update
-**How to find:** `ls configs/ config/ 2>/dev/null` — check file extensions against the format required by the standard. +**How to find:** Discover config-like files repository-wide, then validate extensions against the format required by the standard. +Example: +`find . \( -iname "*.yaml" -o -iname "*.yml" -o -iname "*.json" -o -iname "*.toml" -o -iname "*.ini" \) 2>/dev/null` +Then compare discovered extensions with the standard’s required format.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet-standards/skills/standards-audit/references/configuration-checks.md` around lines 112 - 116, The Check 11 "Config File Format" currently only checks `configs/` and `config/` via the `ls configs/ config/` command and therefore misses config files elsewhere; update the check to discover config files project-wide by replacing the hardcoded ls with a recursive search (e.g., `git ls-files` or `find . -type f -iname '*.yaml' -o -iname '*.yml' -o -iname '*.json' -o -iname '*.toml' -o -iname '*.ini'`) and then filter results against the required format (YAML), excluding vendor/build dirs and dotfiles as needed so non-YAML configs outside config*/ are flagged; make this change where the "Check 11: Config File Format" description and the example discovery command appear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hyperfleet-code-review/skills/review-pr/SKILL.md`:
- Around line 62-75: The loop that iterates over STANDARDS_FILES (for FILE in
$STANDARDS_FILES) decodes each standard but does not record per-file failures,
so partial fetch failures are silent; update that block to detect and record
failures for each FILE (e.g., capture exit status of the gh api and base64 -d
steps), append failed filenames to a failures list or write them to an artifacts
file, and make the script either exit non-zero or emit a summary that Step 5
(intra-PR consistency check) can read to distinguish partial coverage; ensure
the failure variable/list name is unique (e.g., STANDARDS_FETCH_FAILURES) and is
populated inside the loop whenever a fetch or decode fails.
In
`@hyperfleet-standards/skills/standards-audit/references/generated-code-checks.md`:
- Around line 128-132: The "Error Handling" rules reference skipping the
generated code policy when the repository type is Infrastructure or
Documentation but the workflow never establishes repository type; update the
reference implementation to either (a) require and validate an explicit input
named repositoryType (or repo_type) from the orchestrator/agent and use it in
the type-based skip logic, or (b) remove the repo-type skip rule and instead
only rely on detected/generated-code presence and orchestrator-supplied standard
content; make the change where the "Error Handling" section and its three
bullets are defined so the skip condition consistently reads the new
repositoryType input (or is removed) and ensure the spec documents the required
input name and validation behavior for consumers.
In `@hyperfleet-standards/skills/standards-audit/SKILL.md`:
- Around line 41-58: The initial list fetch into STANDARDS_FILES can fail
silently; detect errors from the gh api call that populates STANDARDS_FILES (the
command assigned to STANDARDS_FILES), and if it fails or produces an empty
result when it shouldn't, log a clear error and exit non‑zero or otherwise
surface the failure before entering the for loop; keep the existing
FAILED_STANDARDS collection and loop logic but add an explicit check after
populating STANDARDS_FILES to emit a descriptive "Failed to list standards"
message (including gh error/exit info) and stop the script to avoid silent audit
degradation.
---
Duplicate comments:
In
`@hyperfleet-standards/skills/standards-audit/references/configuration-checks.md`:
- Around line 112-116: The Check 11 "Config File Format" currently only checks
`configs/` and `config/` via the `ls configs/ config/` command and therefore
misses config files elsewhere; update the check to discover config files
project-wide by replacing the hardcoded ls with a recursive search (e.g., `git
ls-files` or `find . -type f -iname '*.yaml' -o -iname '*.yml' -o -iname
'*.json' -o -iname '*.toml' -o -iname '*.ini'`) and then filter results against
the required format (YAML), excluding vendor/build dirs and dotfiles as needed
so non-YAML configs outside config*/ are flagged; make this change where the
"Check 11: Config File Format" description and the example discovery command
appear.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 003c8b24-f555-4627-be0d-2f2b300d40c3
📒 Files selected for processing (30)
hyperfleet-code-review/.claude-plugin/plugin.jsonhyperfleet-code-review/skills/review-pr/SKILL.mdhyperfleet-code-review/skills/review-pr/group-01-error-handling.mdhyperfleet-code-review/skills/review-pr/group-02-concurrency.mdhyperfleet-code-review/skills/review-pr/group-03-exhaustiveness.mdhyperfleet-code-review/skills/review-pr/group-04-resource-lifecycle.mdhyperfleet-code-review/skills/review-pr/group-05-code-quality.mdhyperfleet-code-review/skills/review-pr/group-06-testing.mdhyperfleet-code-review/skills/review-pr/group-07-naming.mdhyperfleet-code-review/skills/review-pr/group-08-security.mdhyperfleet-code-review/skills/review-pr/group-09-code-hygiene.mdhyperfleet-code-review/skills/review-pr/group-10-performance.mdhyperfleet-code-review/skills/review-pr/mechanical-passes.mdhyperfleet-code-review/skills/review-pr/output-format.mdhyperfleet-standards/.claude-plugin/plugin.jsonhyperfleet-standards/skills/standards-audit/SKILL.mdhyperfleet-standards/skills/standards-audit/references/configuration-checks.mdhyperfleet-standards/skills/standards-audit/references/container-image-checks.mdhyperfleet-standards/skills/standards-audit/references/dependency-pinning-checks.mdhyperfleet-standards/skills/standards-audit/references/directory-structure-checks.mdhyperfleet-standards/skills/standards-audit/references/error-model-checks.mdhyperfleet-standards/skills/standards-audit/references/generated-code-checks.mdhyperfleet-standards/skills/standards-audit/references/graceful-shutdown-checks.mdhyperfleet-standards/skills/standards-audit/references/health-endpoints-checks.mdhyperfleet-standards/skills/standards-audit/references/helm-chart-checks.mdhyperfleet-standards/skills/standards-audit/references/linting-checks.mdhyperfleet-standards/skills/standards-audit/references/logging-checks.mdhyperfleet-standards/skills/standards-audit/references/makefile-checks.mdhyperfleet-standards/skills/standards-audit/references/metrics-checks.mdhyperfleet-standards/skills/standards-audit/references/tracing-checks.md
💤 Files with no reviewable changes (1)
- hyperfleet-code-review/skills/review-pr/mechanical-passes.md
✅ Files skipped from review due to trivial changes (13)
- hyperfleet-code-review/.claude-plugin/plugin.json
- hyperfleet-standards/.claude-plugin/plugin.json
- hyperfleet-standards/skills/standards-audit/references/dependency-pinning-checks.md
- hyperfleet-standards/skills/standards-audit/references/directory-structure-checks.md
- hyperfleet-code-review/skills/review-pr/group-05-code-quality.md
- hyperfleet-code-review/skills/review-pr/group-03-exhaustiveness.md
- hyperfleet-code-review/skills/review-pr/group-07-naming.md
- hyperfleet-code-review/skills/review-pr/group-08-security.md
- hyperfleet-code-review/skills/review-pr/group-04-resource-lifecycle.md
- hyperfleet-standards/skills/standards-audit/references/container-image-checks.md
- hyperfleet-code-review/skills/review-pr/group-06-testing.md
- hyperfleet-code-review/skills/review-pr/group-01-error-handling.md
- hyperfleet-code-review/skills/review-pr/group-02-concurrency.md
🚧 Files skipped from review as they are similar to previous changes (9)
- hyperfleet-code-review/skills/review-pr/output-format.md
- hyperfleet-standards/skills/standards-audit/references/linting-checks.md
- hyperfleet-standards/skills/standards-audit/references/metrics-checks.md
- hyperfleet-standards/skills/standards-audit/references/makefile-checks.md
- hyperfleet-standards/skills/standards-audit/references/tracing-checks.md
- hyperfleet-standards/skills/standards-audit/references/logging-checks.md
- hyperfleet-standards/skills/standards-audit/references/health-endpoints-checks.md
- hyperfleet-standards/skills/standards-audit/references/error-model-checks.md
- hyperfleet-standards/skills/standards-audit/references/helm-chart-checks.md
hyperfleet-standards/skills/standards-audit/references/generated-code-checks.md
Show resolved
Hide resolved
Contributor
Nit: Inconsistent capitalization in "standard unavailable" messagesThe mandatory finding messages emitted when a standard fetch fails vary in capitalization across the group files:
Since these show up as visible findings in the review output, it would be worth picking one convention (I'd suggest Title Case for the standard names, e.g. "required Error Model standard unavailable") and applying it consistently. |
7 tasks
…audit and review-pr - Replace slow multi-Skill-call standards fetch (14+ individual calls to hyperfleet-architecture) with direct batch fetch via gh api - Remove hyperfleet-architecture skill dependency for standards fetching - Update error handling references accordingly
…ssages Use Title Case for standard names consistently across all group files.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
hyperfleet-code-review/skills/review-pr/SKILL.md (1)
245-245:⚠️ Potential issue | 🟠 MajorChecklist still contradicts the standards-unavailable path.
Line 245 is unconditional (
Intra-PR consistency checked against HyperFleet standards), but Line 131 explicitly defines a skip mode when standards are unavailable. The checklist should allow “skipped/NA” in that case.Suggested fix
-- [ ] Intra-PR consistency checked against HyperFleet standards +- [ ] Intra-PR consistency checked against HyperFleet standards (or marked skipped/NA when standards are unavailable)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet-code-review/skills/review-pr/SKILL.md` at line 245, The checklist entry "Intra-PR consistency checked against HyperFleet standards" is unconditional but must respect the skip mode declared at the earlier "standards-unavailable" path (refer to the logic around Line 131); update the checklist item so it allows a skipped/NA state (e.g., "Intra-PR consistency checked against HyperFleet standards (or skipped/NA if standards unavailable)") and ensure any guidance text references the standards-unavailable skip mode to keep behavior consistent with the existing skip logic.hyperfleet-code-review/skills/review-pr/group-09-code-hygiene.md (1)
7-7:⚠️ Potential issue | 🟡 MinorNormalize the fallback finding string to the canonical standard name.
Line 7 uses
"required Commit Message standard unavailable"(lowercasestandard), while other groups/messages use canonical Title Case names. This inconsistency can cause drift in user-visible findings and standards matching behavior.Suggested fix
-... emit a mandatory finding stating "required Commit Message standard unavailable" ... +... emit a mandatory finding stating "required Commit Message Standard unavailable" ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet-code-review/skills/review-pr/group-09-code-hygiene.md` at line 7, The fallback finding string is inconsistent in casing; replace the literal "required Commit Message standard unavailable" with the canonical Title Case string (e.g., "Required Commit Message Standard Unavailable") wherever it appears in the file (look for the string in group-09-code-hygiene.md and any related message templates), and normalize any other occurrences or comparisons that match this message so findings and standards matching use the exact canonical name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hyperfleet-code-review/skills/review-pr/group-10-performance.md`:
- Around line 20-27: Pass 10b currently duplicates group-04 checks by flagging
defer-in-loop patterns that Pass 4a/4c already evaluate; update the Pass 10b
rule so it first consults group-04 outcomes (Pass 4a and Pass 4c) or a shared
metadata/tagging mechanism and only emit findings when (a) the defer is in a
tight loop that will accumulate across many iterations or (b) an N+1 pattern is
detected (per-item DB/API call) — otherwise skip reporting to avoid
conflicting/duplicate findings; implement this by adding a pre-check in the Pass
10b evaluator to query the resource-lifecycle verdicts from Pass 4a/4c (or a
centralized rule registry) and only run the tight-loop/N+1 heuristics if no
prior cleanup/exception justification exists.
In `@hyperfleet-code-review/skills/review-pr/SKILL.md`:
- Around line 139-140: Update the bullet that currently reads "*[standards]*
Naming conventions, logging patterns, config access patterns — compare against
logging specification standard" so it references the fetched/relevant standards
rather than the Logging Specification; locate the exact bullet text in SKILL.md
and change "logging specification standard" to something like "the relevant
fetched standard(s)" (or a variable/placeholder that points to the retrieved
standards), ensuring the rule applies to naming conventions, logging patterns
and config access patterns against the appropriate standard source.
---
Duplicate comments:
In `@hyperfleet-code-review/skills/review-pr/group-09-code-hygiene.md`:
- Line 7: The fallback finding string is inconsistent in casing; replace the
literal "required Commit Message standard unavailable" with the canonical Title
Case string (e.g., "Required Commit Message Standard Unavailable") wherever it
appears in the file (look for the string in group-09-code-hygiene.md and any
related message templates), and normalize any other occurrences or comparisons
that match this message so findings and standards matching use the exact
canonical name.
In `@hyperfleet-code-review/skills/review-pr/SKILL.md`:
- Line 245: The checklist entry "Intra-PR consistency checked against HyperFleet
standards" is unconditional but must respect the skip mode declared at the
earlier "standards-unavailable" path (refer to the logic around Line 131);
update the checklist item so it allows a skipped/NA state (e.g., "Intra-PR
consistency checked against HyperFleet standards (or skipped/NA if standards
unavailable)") and ensure any guidance text references the standards-unavailable
skip mode to keep behavior consistent with the existing skip logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 758bd9be-c3ff-4d57-9692-c193e4043d08
📒 Files selected for processing (30)
hyperfleet-code-review/.claude-plugin/plugin.jsonhyperfleet-code-review/skills/review-pr/SKILL.mdhyperfleet-code-review/skills/review-pr/group-01-error-handling.mdhyperfleet-code-review/skills/review-pr/group-02-concurrency.mdhyperfleet-code-review/skills/review-pr/group-03-exhaustiveness.mdhyperfleet-code-review/skills/review-pr/group-04-resource-lifecycle.mdhyperfleet-code-review/skills/review-pr/group-05-code-quality.mdhyperfleet-code-review/skills/review-pr/group-06-testing.mdhyperfleet-code-review/skills/review-pr/group-07-naming.mdhyperfleet-code-review/skills/review-pr/group-08-security.mdhyperfleet-code-review/skills/review-pr/group-09-code-hygiene.mdhyperfleet-code-review/skills/review-pr/group-10-performance.mdhyperfleet-code-review/skills/review-pr/mechanical-passes.mdhyperfleet-code-review/skills/review-pr/output-format.mdhyperfleet-standards/.claude-plugin/plugin.jsonhyperfleet-standards/skills/standards-audit/SKILL.mdhyperfleet-standards/skills/standards-audit/references/configuration-checks.mdhyperfleet-standards/skills/standards-audit/references/container-image-checks.mdhyperfleet-standards/skills/standards-audit/references/dependency-pinning-checks.mdhyperfleet-standards/skills/standards-audit/references/directory-structure-checks.mdhyperfleet-standards/skills/standards-audit/references/error-model-checks.mdhyperfleet-standards/skills/standards-audit/references/generated-code-checks.mdhyperfleet-standards/skills/standards-audit/references/graceful-shutdown-checks.mdhyperfleet-standards/skills/standards-audit/references/health-endpoints-checks.mdhyperfleet-standards/skills/standards-audit/references/helm-chart-checks.mdhyperfleet-standards/skills/standards-audit/references/linting-checks.mdhyperfleet-standards/skills/standards-audit/references/logging-checks.mdhyperfleet-standards/skills/standards-audit/references/makefile-checks.mdhyperfleet-standards/skills/standards-audit/references/metrics-checks.mdhyperfleet-standards/skills/standards-audit/references/tracing-checks.md
💤 Files with no reviewable changes (1)
- hyperfleet-code-review/skills/review-pr/mechanical-passes.md
✅ Files skipped from review due to trivial changes (14)
- hyperfleet-standards/.claude-plugin/plugin.json
- hyperfleet-standards/skills/standards-audit/references/dependency-pinning-checks.md
- hyperfleet-standards/skills/standards-audit/references/directory-structure-checks.md
- hyperfleet-code-review/.claude-plugin/plugin.json
- hyperfleet-code-review/skills/review-pr/group-03-exhaustiveness.md
- hyperfleet-code-review/skills/review-pr/output-format.md
- hyperfleet-code-review/skills/review-pr/group-05-code-quality.md
- hyperfleet-code-review/skills/review-pr/group-02-concurrency.md
- hyperfleet-code-review/skills/review-pr/group-06-testing.md
- hyperfleet-code-review/skills/review-pr/group-01-error-handling.md
- hyperfleet-code-review/skills/review-pr/group-08-security.md
- hyperfleet-code-review/skills/review-pr/group-04-resource-lifecycle.md
- hyperfleet-code-review/skills/review-pr/group-07-naming.md
- hyperfleet-standards/skills/standards-audit/references/metrics-checks.md
🚧 Files skipped from review as they are similar to previous changes (3)
- hyperfleet-standards/skills/standards-audit/references/makefile-checks.md
- hyperfleet-standards/skills/standards-audit/references/linting-checks.md
- hyperfleet-standards/skills/standards-audit/references/tracing-checks.md
Comment on lines
+20
to
+27
| List every `defer` statement in the diff. For each, check: | ||
|
|
||
| - **Defer in tight loops** — flag `defer` inside `for` loops, as deferred calls accumulate until the function returns, not per iteration. Suggest extracting the loop body into a separate function or calling cleanup explicitly | ||
| - **N+1 query patterns** — in code that iterates over a collection, flag individual database/API calls per item when a batch operation is available (e.g., fetching one record at a time in a loop instead of a single query with `WHERE IN`) | ||
|
|
||
| Do NOT flag: | ||
| - `defer` in functions that return after the loop (single iteration patterns) | ||
| - Loops with very small, bounded iteration counts (e.g., iterating over 2-3 known items) |
There was a problem hiding this comment.
Pass 10b overlaps with existing resource-lifecycle checks (group-04).
Pass 10b flags defer inside loops, but group-04 already covers defer patterns:
- Pass 4a requires defer for resource cleanup (which may legitimately occur in loops)
- Pass 4c specifically checks
time.Aftermemory leaks in loops
This creates potential for conflicting or duplicate findings across the two groups. For example, a defer used for proper cleanup in a loop could be flagged here as a performance issue while being required by Pass 4a.
Consider either:
- Narrowing Pass 10b to only flag defer when Pass 4a's cleanup requirements are already satisfied elsewhere, or
- Adding explicit coordination guidance between the two groups
🧰 Tools
🪛 LanguageTool
[style] ~27-~27: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...single iteration patterns) - Loops with very small, bounded iteration counts (e.g., iterat...
(EN_WEAK_ADJECTIVE)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hyperfleet-code-review/skills/review-pr/group-10-performance.md` around lines
20 - 27, Pass 10b currently duplicates group-04 checks by flagging defer-in-loop
patterns that Pass 4a/4c already evaluate; update the Pass 10b rule so it first
consults group-04 outcomes (Pass 4a and Pass 4c) or a shared metadata/tagging
mechanism and only emit findings when (a) the defer is in a tight loop that will
accumulate across many iterations or (b) an N+1 pattern is detected (per-item
DB/API call) — otherwise skip reporting to avoid conflicting/duplicate findings;
implement this by adding a pre-check in the Pass 10b evaluator to query the
resource-lifecycle verdicts from Pass 4a/4c (or a centralized rule registry) and
only run the tight-loop/N+1 heuristics if no prior cleanup/exception
justification exists.
Comment on lines
+139
to
+140
| - *[standards]* Naming conventions, logging patterns, config access patterns — compare against logging specification standard | ||
| - *[standards]* Flag deviations from team standards — if the PR introduces a pattern that contradicts a HyperFleet standard, flag it |
There was a problem hiding this comment.
Use the correct standards scope for consistency checks.
Line 139 currently says naming conventions and config access patterns should be compared against the Logging Specification. That narrows validation to the wrong source and can misclassify findings. This should reference the relevant fetched standard(s), not logging only.
Suggested fix
-- *[standards]* Naming conventions, logging patterns, config access patterns — compare against logging specification standard
+- *[standards]* Naming conventions, logging patterns, config access patterns — compare against the relevant HyperFleet standard(s) fetched in step 4b🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hyperfleet-code-review/skills/review-pr/SKILL.md` around lines 139 - 140,
Update the bullet that currently reads "*[standards]* Naming conventions,
logging patterns, config access patterns — compare against logging specification
standard" so it references the fetched/relevant standards rather than the
Logging Specification; locate the exact bullet text in SKILL.md and change
"logging specification standard" to something like "the relevant fetched
standard(s)" (or a variable/placeholder that points to the retrieved standards),
ensuring the rule applies to naming conventions, logging patterns and config
access patterns against the appropriate standard source.
Contributor
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rh-amarin The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit 60df807
into
openshift-hyperfleet:main
2 checks passed
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
4 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
mechanical-passes.mdinto 10 modular group files with sequential IDs (1a–10b)hyperfleet-architectureskill in both pluginsgenerated-code-checks.mdwith 7 checksVersion bumps
hyperfleet-code-review0.3.0 → 0.4.0hyperfleet-standards1.0.0 → 1.1.0Test plan
hyperfleet-standardsplugin and run/standards-auditon a component repo — verify all 14 standards are audited with no hardcoded valueshyperfleet-code-reviewplugin and run/review-pron a PR — verify mechanical passes load from individual group files and defer to fetched standardsgenerated-code-checks.mdis picked up for repos with generated codeSummary by CodeRabbit
New Features
Improvements
Chores