Repository navigation
Conversation
…nistic correction IDs
State machines (CaaS + VMaaS):
- CaaS: replace default updated.v1 with explicit non-billable→billable
as resumed.v1. Default case now errors on unexpected transitions.
- VMaaS: enumerate all known states explicitly (STARTING, STOPPING,
DELETING, DELETE_FAILED, UNSPECIFIED). Default case errors.
NodeSet-keyed components:
- Add NodeSet field to ComponentRecord, populated from spec.node_sets
map key. Prevents host_type collision when two pools share a type.
- ComponentEventID uses NodeSet as unique key: {eventID}/{nodeSet}
- ChangedComponents keys on NodeSet instead of component:host_type
- FlatBillingDimensions includes node_set field
Deterministic correction event IDs:
- Derive base ID from resourceID + reason + states instead of
uuid.NewString(). Same drift across reconciliation cycles produces
same event IDs, enabling adapter-level dedup.
Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
- correctionDescription returns error for unknown CorrectionReason - isBillableForType returns error for unknown resource type - No silent fallbacks remain in any switch statement Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
…, proto compat - ChangedComponents: preserve NodeSet on removed component records so ComponentEventID produces unique IDs and FlatBillingDimensions carries the correct node_set for billing closure - buildSyntheticHeartbeats: derive deterministic base ID from resourceID + timestamp instead of uuid.NewString, enabling adapter dedup on retry - Adapt to typed proto references (ClusterTemplateReference, ClusterCatalogItemReference, HostTypeReference, VersionName) - Fix goimports formatting Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
…tables State transitions are now defined as maps — the table IS the spec. Missing entry = error (fail fast). No default branches to get wrong. Adding BMaaS or any new resource type = adding a new table. VMaaS (compute_instance): transient states (STOPPING/STARTING), DELETING as billing-ending, explicit wildcard fallbacks. CaaS (cluster_order): billable-to-billable skip, non-billable skip, same-state transitions for scaling, explicit per-state entries. Shared lookup in transitions.go: exact (from, to) match first, then (*,to) wildcard, then error. Signed-off-by: omer-vishlitzky <ovishliz@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
Fixes review findings from 5-agent critical review: 1. publishAndUpsert: all create/update paths now publish events BEFORE committing projection state. If Kafka fails, projection is not committed, replay retries full publish. Prevents permanent event loss on partial N+1 publish failure. Delete path was already correct (publish-first from PR 135 review). 2. Missing initial CaaS transitions: added ""->FAILED/DELETING/ DELETE_FAILED/UNSPECIFIED as Skip. Prevents consumer crash when first-observed cluster is in non-billable state (bootstrap, reconnect after failure, existing deployment). 3. ErrSkipNonBillingTransition renamed to ErrSkipTransition — the old name was misleading for billable-to-billable transitions (PROGRESSING->READY). 4. Strengthened multi-removal test: verifies exact NodeSet->HostType preservation, not just non-empty NodeSet. 5. DimensionsEqual component ordering test: documents that array order matters (ClusterBillingDimensions sorts keys for determinism). 6. Proto enumeration completeness test: iterates every (from, to) state pair from all ClusterState proto values plus empty initial. If someone removes a table entry, this test fails — prevents wildcard masking. 7. Stale version test updated: publish-first means events reach Kafka even when projection upsert is skipped (adapter dedup handles it). Signed-off-by: omer-vishlitzky <ovishliz@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
Addresses amito's review comment: the N+1 decompose-build-publish pattern repeated across publishLifecycleEvents, buildCorrectionEvents, buildSyntheticHeartbeats, and buildHeartbeatEvents. DecomposeClusterEvents handles only cluster_order — no fallback. Empty components returns ErrDataQuality (fail fast, not silent single-event degradation). Callers branch on resource type and call DecomposeClusterEvents only for cluster_order. Signed-off-by: omer-vishlitzky <ovishliz@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
…rals Zero switch statements in production code. Zero wildcards. Zero hardcoded string literals. All state machines, event types, resource types, timestamps, corrections, and decomposition use map-based dispatch with constants. Missing entry = error, no fallback. Changes: - Constants for all event types (EventCreated, EventSuspended, ...), resource types (ResourceTypeComputeInstance, ResourceTypeClusterOrder), VMaaS states (CIState*), CaaS states (CLState*) - ResolveCloudEventType: map-based, replaces identical switch in both mappers. CREATED/DELETED fixed, UPDATED delegates to transition table - ResolveTransitionTime: map-based, replaces identical switch in both mappers. Unknown event type = error, nil timestamp = ErrDataQuality - BuildResourceEvents: map-based decomposition dispatch, replaces 5 separate "if cluster_order" branches. Unknown resource type = error. Adding BMaaS = one line in the map - VMaaS transition table: 72 explicit entries (9 from × 8 to), no wildcards. Fixes latent bugs: ""→STOPPED was suspended.v1 (should be skip, no interval to close), RUNNING→UNSPECIFIED was updated.v1 (should be suspended.v1, billable→non-billable) - correctionDescription: map replaces switch, unknown reason = error - isBillableForType: map replaces switch, unknown type = false 318 specs pass (was 240). New test files: transitions_test.go (18 specs), correction_internal_test.go (5 specs). VMaaS exhaustive DescribeTable (72 entries) + proto enumeration completeness test. Signed-off-by: omer-vishlitzky <ovishliz@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
…nsumer
The Watch Consumer crashed and reconnected on every OBJECT_SIGNALED
event (7 per E2E run) and every metadata-only OBJECT_UPDATED without
state_transition_time (3 per run). Each crash lost the bad event and
all subsequent events on that stream, since the Watch protocol has
no resume token.
Two targeted fixes — everything else still fails fast:
1. SIGNALED: ResolveCloudEventType and ResolveTransitionTime return
ErrUnsupportedEvent. The consumer skips the event, increments
osac_metering_events_skipped_total{reason=unsupported_event_type},
and continues processing the same stream.
2. Metadata-only updates: when TransitionTime returns ErrDataQuality
but the resource state hasn't changed (same as projection), the
consumer skips silently — no state transition occurred, nothing
to meter.
Real data quality issues (state changed but no timestamp, missing
resource_id, missing tenant_id) still crash the stream so they
surface as fulfillment-service bugs.
Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
Finding 1 (CRITICAL): VMaaS dimension change was silently dropped. handleScalingEvent called ChangedComponents (CaaS-specific), returned empty for VMaaS, updated projection without publishing. Fixed with BuildDimensionChangeEvents map-based dispatch: VMaaS emits single updated.v1, CaaS emits per-changed-component events. Also fixed ChangedComponents to compare HostType (not just NodeCount). Finding 2 (HIGH): billing dimension field renamed from version_name to release_image, matching the design doc and CaaS events spec gist adapter contract. Finding 3 (MEDIUM): cluster_order created.v1/deleted.v1 now have flat billing_dimensions (cluster_template, release_image only), no nested components array. Matches design spec for audit events. Finding 4 (MEDIUM): non-billable→billable transitions now consistently use resumed.v1 across both VMaaS and CaaS. Only initial ""→billable uses started.v1. Eliminates adapter special-casing between resource types. Signed-off-by: omer-vishlitzky <ovishliz@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
MapWatchEvent now takes billingDims as a parameter instead of calling mapper.BillingDimensionsMap() internally. Callers pass the right dims for their context: - Audit events (created/deleted): topLevelDims() strips components BEFORE building the CloudEvent (was: build with nested, then deserialize-strip-reserialize after) - Lifecycle events: dims passed through, replaced per-component during decomposition anyway Removes stripComponentsFromAuditEvent (serialize-deserialize round-trip). Signed-off-by: omer-vishlitzky <ovishliz@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: omer-vishlitzky <omer.vishlitzky@gmail.com>
…view
ChangedComponents compared only NodeCount and HostType, so a cluster
upgrade (release_image change) or template change with unchanged
topology was silently dropped: no updated.v1 published, and the
projection still absorbed the new dimensions unconditionally in
handleScalingEvent — which permanently defeats the reconciler's
billing_dimensions_drift safety net, since the projection now matches
fulfillment and no drift is ever detected. Fixed by comparing via
DimensionsEqual on FlatBillingDimensions, the same equality already
used to gate entry into this code path, so the two can't disagree.
Two ID-derivation bugs meant retries didn't dedup the way CAP-16
("duplicate events do not cause double-counting") requires:
- Correction event IDs were derived from resourceID/reason/states only,
with no discriminator. Two distinct billing_dimensions_drift
corrections for the same resource/state collided on ID, so adapter
dedup silently dropped the second one. Now includes a content
fingerprint of the billing dimensions, so distinct drifts get
distinct IDs while repeat detection of the *same* unresolved drift
still dedups as designed.
- Reconciler's synthetic heartbeat IDs were keyed off "now", so a retry
in a later reconciliation cycle for the same unresolved staleness gap
minted a fresh ID for components already delivered in a prior partial
failure. Now keyed off LastHeartbeatAt/BillableSince — the actual
reference point for the gap — so retries of the same gap dedup.
Heartbeat Generator: base ID now keyed to the heartbeat window instead
of a random UUID (consistency/future-proofing for the ID scheme, not
an active bug — each real tick is a legitimately new heartbeat either
way). More importantly, one resource's publish failure no longer
aborts the whole tick; it's isolated so other resources still get
heartbeated.
Also:
- buildComponentEvent guards against a nil base-event data map
(DataAs leaves the target nil, not an error, when data is empty)
- node_count is validated (integral, non-negative, in int32 range)
before narrowing; a corrupt value now fails the whole decomposition
loudly instead of either silently truncating/wrapping the billed
value or (an earlier draft of this fix) silently dropping just the
bad component, which would have been a new, differently-shaped
silent data loss
- Unified the duplicated lifecycle/scaling event payload construction
(meteringData / buildScalingEvent's hand-rolled map) into one
exported LifecycleData/BuildLifecycleData used by both, so the two
producers can't drift into different shapes on the same topic
- Removed the dead StateContext.NewDimensions field (assigned, never read)
Assisted-by: Claude Code <noreply@anthropic.com>
…idation gaps from review - Bump private-api pin to v0.0.84 and regenerate: fulfillment-service's version_name->ClusterVersionReference conversion made metering's stale proto silently decode the wrong field. Read spec.GetVersion().GetName(). - Track per-component billable-since (ComponentBillableSince) instead of one cluster-wide timestamp, so staggered scaling of independent node sets no longer understates duration_seconds for whichever component didn't cause the most recent reset. Threaded through Reconciler's missed_creation/state_drift paths too, since they write projection rows directly. Folded into the initial migration (no live database yet). - Remove node_count validation from DecomposeClusterComponents entirely: fulfillment-service already rejects non-positive node set sizes at the API layer, and ClusterBillingDimensions always writes a valid typed value, so the check (and JSONB-corruption-only failure mode a stricter version of it would have added) defended against a scenario that can't occur through any real code path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…etalInstance create When network_attachments is omitted during BareMetalInstance creation, auto-populate with the tenant's default IPv4 Subnet and default SecurityGroup (labeled osac.openshift.io/default), using the first fabric-role interface from the HostType. If no defaults exist, the field remains empty (graceful skip). Adds securityGroupsDao and tenancyLogic to the BM server struct for default resource lookup and tenant resolution. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
… swallowing DAO errors in the catalog → template → host_type chain were silently swallowed, causing the interface field to be silently omitted on transient DB failures. Now NotFound errors are treated as "chain unresolvable" (return empty, no error) while other errors propagate as Internal, consistent with findDefaultSubnet/findDefaultSecurityGroup. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
New task file that queries the Netris Controller DHCP lease API for a subnet, matches a server port MAC address to find the DHCP-assigned IP, and returns it via a discovered_ip_address fact. Used by the bare-metal-fulfillment-operator during reconcileIPDiscovery after provisioning completes. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
The query_dhcp_lease role is generic — it works for any resource type that receives IPs via fabric DHCP, not just bare-metal instances. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
…ller API
The Netris controller does not expose a /api/v2/dhcp lease endpoint.
DHCP is served by Kea on softgate nodes, one instance per VPC (VRF).
Replace the controller API query with direct Kea control socket queries
on softgates: resolve Subnet → V-Net → VPC ID → VRF name, discover
softgates via hardware inventory API, then SSH to each and query
lease4-get-by-hw-address on /tmp/kea4-Vrf_{vpc_id}-ctrl-socket.
Tested on netris-lab with a live DHCP lease (server h03 ens5 on
subnet-m6vzc V-Net, VPC 3 / VRF Vrf_3).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add MAC format validation (assert) before shell interpolation to prevent injection via crafted MAC values - Fix IP concatenation bug: use loop with early-exit guard instead of Jinja2 template that concatenated IPs from multiple softgates - Add no_log: true to Kea query and response parsing tasks Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
Rewrite DHCP lease query to use GET /api/v2/ipam/hosts/{subnetID}
per Netris team guidance. This replaces the SSH-to-softgate Kea
approach with a clean REST API call that follows existing patterns
(same endpoint used by netris.controller.ipam.query_subnet_hosts).
Resolves all SSH-related review concerns (StrictHostKeyChecking,
shell injection surface, socat dependency, from_json safety).
Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Dan Manor <dmanor@redhat.com>
Tested against live Netris controller — DHCP lease entries have a
mac array (not meta.mac): [{address, source, state}]. Updated the
matching logic to iterate item.mac[].address.
Assisted-by: Claude Code <noreply@anthropic.com>
Signed-off-by: Dan Manor <dmanor@redhat.com>
…command Add --network-attachment flag to the BareMetalInstance CLI create command, parsing interface, subnet, security-groups, and primary fields into BareMetalNetworkAttachment proto messages. Multiple flags can be specified for multi-homed bare metal instances. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
…parsing Table-driven Ginkgo tests covering parseBareMetalNetworkAttachmentFlag (valid and invalid inputs), applyNetworkingFlags (populated and empty), and flag registration. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
…ent paths Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
…ackage Move the security-groups parsing function to netutil and fix a case inconsistency: the old implementation lowercased the prefix and group values when security groups were present but preserved case when they were absent. The shared version uses original-string byte offsets so case is always preserved. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
…ce and types Signed-off-by: Amit Oren <amoren@redhat.com>
- Change go.work from 1.26.4 to 1.26.3 to match global constraint - Change osac-metering/adapters/go.mod from 1.26.4 to 1.26.3 - Run go mod tidy to add missing github.com/cloudevents/sdk-go/v2 v2.16.2 - All transitive dependencies now properly resolved Signed-off-by: Amit Oren <amoren@redhat.com>
Signed-off-by: Amit Oren <amoren@redhat.com>
Signed-off-by: Amit Oren <amoren@redhat.com>
Signed-off-by: Amit Oren <amoren@redhat.com>
Reverses the test-directory excludes added when the corpus scope was first defined. Those excludes were based on general third-party guidance (tests bloat node count 3-4x with no new edges) that was never actually measured against this repo. Real usage since found genuine value in 'which tests cover this function' questions and source-to-test edges, which the current exclude-tests scope can't answer at all. Real measured impact (node/link count, load time) to follow in the PR description once a live workflow_dispatch run completes.
fulfillment-service is PostgreSQL-backed, so the brain was blind to SQL schema/migrations without this -- confirmed via the first production run's own warning (109 .sql files skipped, tree_sitter_sql not installed). graphifyy[sql] is a real PyPI extra; verified locally that pip resolves it correctly and pulls in tree-sitter-sql.
…er code to proto layer Co-authored-by: Zoltan Szabo <zszabo@redhat.com>
Co-authored-by: Zoltan Szabo <zszabo@redhat.com>
Co-authored-by: Zoltan Szabo <zszabo@redhat.com>
Co-authored-by: Zoltan Szabo <zszabo@redhat.com>
Co-authored-by: Zoltan Szabo <zszabo@redhat.com>
Add create and describe commands for BareMetalInstanceType: - create baremetalinstancetype: Creates BareMetalInstanceType with hardware specs - describe baremetalinstancetype: Shows detailed hardware specifications - get baremetalinstancetype: List and retrieve instances (via reflection system) Includes comprehensive test coverage and integration with CLI framework. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Austin Jamias <ajamias@redhat.com>
…nstall wiring Adds a SessionStart hook script that pulls the latest CI-published graphify bundle (OSAC-4016) into graphify-out/ at the start of every session: binary-missing check, staleness signal via merge-base ancestry (TTL-based note when unavailable, never a hard rejection), version check against metadata.json with an exact upgrade command on mismatch, full validation of both graph.json and metadata.json before touching local state, and an atomic temp-dir-then-mv swap. Every failure path falls back to whatever's already local (naming its known SHA/age) or signals brain unavailable, and the script always exits 0 -- it must never block session start. Registered as a second SessionStart command alongside the existing update-ai-context.sh hook, not a replacement. graphify claude install itself (the CLAUDE.md directive + PreToolUse hook that make Claude Code actually consult the graph) has not been run here -- no graphify binary was available to generate and verify its real output. Documented as a required one-time follow-up in AGENTS.md instead of hand-fabricating what that command would write.
- Fix the version-comparison bug that would have refused every fetch: graphify --version prints non-numeric text, so strip to the bare numeric version with grep -oE on both the fetch script and (paired fix in OSAC-4016) the workflow that writes graphify_version. - Report git-relative staleness (commits behind) when the bundle's source SHA is an ancestor of local HEAD, not just when it isn't. - Switch the atomic swap to rename-based (old dir kept as .old until the new one is fully in place) instead of delete-then-replace, closing the window where graphify-out/ could be entirely missing if the hook is killed mid-swap. - Add a tar-listing path-traversal/symlink guard before extracting the downloaded bundle. - Anchor the script to CLAUDE_PROJECT_DIR internally and drop the cd wrapper from settings.json, matching the existing hook's invocation style. - Reword AGENTS.md to not imply the fetch works with no setup at all -- it still requires graphify to be installed first. - Clarify the fetch comment: gh needs no auth for this public repo, but may pick up an ambient local token if present.
… review) Version check previously refused to load on ANY mismatch, including a local graphify newer than what CI used -- backwards, and the 'upgrade' messaging made no sense in that direction. Use sort -V to determine which side is actually older; only refuse+recommend-upgrade when local is behind, just log a note when local is ahead. Symlink/hardlink detection in the tar-safety guard matched on tar tvzf's human-readable '-> '/'link to' text, which could false-positive on a filename containing those literal substrings. Verified GNU tar's actual listing format directly (real 'l'/'h' leading type characters, same convention as ls -l) and match on that instead.
…es/edges) This was identified and locally patched during the original local-testing round but the fix was never actually committed -- an oversight caught only by this round's stronger test (running the real fetch script against the real published graph-latest bundle instead of synthetic fixtures). The real graph.json is NetworkX node-link JSON format (nodes/links), not nodes/edges, so validation was rejecting every real bundle permanently. Confirmed fixed by actually fetching and swapping in the real 85K-node bundle end to end, not just re-reading the diff.
…ning fixes Required fix: the rename-based swap didn't check mv exit status. Without -e, if the final mv (.tmp -> live) failed after the existing graph had already been moved to .old, the trailing rm -rf .old still ran and deleted the only surviving copy -- exactly the failure mode the rename-based swap was supposed to prevent. Each mv is now checked explicitly; a failed final mv restores .old back into place before falling back. Verified all 3 failure points (stage/preserve/activate) with a mocked failing mv, not just read -- the critical case (final mv fails after old content already relocated) correctly restores the original content rather than leaving graphify-out/ missing. Also: an ERR trap as a belt-and-suspenders safety net for anything not already explicitly guarded; a post-extraction find -type l check (independent of tar's listing-format parsing) plus --no-same-owner on extraction, as a second symlink-detection layer; and a numeric guard on .last-fetch-at's content so a corrupted timestamp file can't produce a bogus age display.
…t idempotent) Co-authored-by: Alexander Chuzhoy <achuzhoy@redhat.com>
…mption Ran graphify claude install for real against this repo (Phase 1's own scratch/local test copies were the only place this had run before) and committed its output so the whole team inherits the consumption side by default, matching how this repo's other shared Claude Code config is committed. Read the actual generated output rather than assuming it would match earlier scratch tests, and found two real problems to fix before committing: - The generated PreToolUse hook commands hardcoded an absolute, machine-specific graphify binary path from the install environment -- would have been broken for every developer except whoever ran the install. Changed to bare `graphify`, resolved via PATH like every other tool invocation in these hooks. - The generated CLAUDE.md directive told agents to run `graphify update .` locally after every code change -- directly contradicts this repo's own already-documented policy (generation centralized in CI, no local regeneration that could clobber the CI-fetched graph). Replaced that line with an explicit pointer to the existing policy instead. Confirmed no clobbering of the existing SessionStart hooks (update-ai-context.sh, fetch-graphify-brain.sh) -- install only added a new PreToolUse block alongside them. Also confirmed the resulting matchers/commands (Bash|Grep -> hook-guard search, Read|Glob -> hook-guard read) match what was seen in earlier scratch testing. Updated AGENTS.md's Knowledge Graph section to drop the now-stale 'has not been run against this repo yet' note.
The PreToolUse hooks installed by 'graphify claude install' (without --strict, not used here) are advisory nudges -- they inject a reminder into context but don't block a raw file read if Claude Code proceeds anyway. Only --strict mode (which actively denies the first raw read of a session) enforces that. Reworded to describe the actual configured behavior instead of implying enforcement.
The PreToolUse hooks invoked graphify hook-guard as bare commands with no existence check -- if graphify isn't installed, every single Bash/Grep/Read/Glob call would throw a visible exit-127 hook error, against this feature's fail-open design everywhere else (the SessionStart fetch hook already does this correctly). Guarded both with command -v graphify first. Used 'command -v graphify || exit 0; graphify hook-guard ...' rather than the more obvious 'command -v graphify && graphify hook-guard ... || true' -- verified empirically that the &&/|| form silently swallows a nonzero exit from hook-guard itself (bash's (A && B) || C precedence), which would defeat any future exit-code-based signal from hook-guard (e.g. if --strict mode is ever enabled). The sequenced form preserves hook-guard's own exit code and stdout untouched when graphify is present, and exits clean 0 with no output when it's not -- tested both cases for real. Also added an explicit 10s timeout to both PreToolUse entries -- they previously defaulted to 600s, which could block a tool call for up to 10 minutes on a hang, against the short timeouts already used for this feature's SessionStart hooks (20s, 30s).
Merge queue previously always ran full E2E because should-run short-circuited for non-pull_request events. Filter both PR and merge_group; keep schedule/dispatch always-on and gate jobs. Assisted-by: Cursor <noreply@cursor.com>
…scope, sync cli-ux verb list Add .claude/rules/protobuf-conventions.md at fulfillment-service scope (was workspace-only in osac-workspace despite being fulfillment-service- specific content) as a short pointer to AGENTS.md's API Design Guidelines and docs/API.md, rather than a restatement. Fix cli-ux.md's stale/inaccurate verb list and path qualifier: - Add the missing scale/tenant verbs (both real internal/cmd/cli/ subcommands, already present in osac-workspace's soon-to-be-retired copy). - Remove lookup, which was wrong in *both* copies -- verified against root_cmd.go's actual AddCommand calls and a real `--help` run: lookup.Find() is an internal helper consumed by scale/get/describe, never a registered top-level command. - Correct the intro line's path qualifier to a bare internal/cmd/cli/, appropriate for a file that already lives inside fulfillment-service. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
… pointer Review follow-up: fulfillment-service/.claude/rules/protobuf-conventions.md turned out to be almost entirely redundant with AGENTS.md once cross-checked line by line -- its buf-lint reminder was stale (AGENTS.md already documents the correct `uv run dev.py lint proto` invocation, which is what CodeRabbit's review flagged this file for missing), its SERVICE_SUFFIX note was already verbatim in AGENTS.md, and its AGENTS.md/docs/API.md pointer duplicated a path this repo's own "Where to Find Information" table already provides directly. The one non-duplicated sentence (OSAC's protobuf conventions being adapted from the Kubernetes API conventions doc) is folded into AGENTS.md's existing "API Design Guidelines" section instead of keeping a second, driftable copy. cli-ux.md is the opposite case -- it holds real, non-duplicated CLI design guidance -- but was missing from AGENTS.md's "Where to Find Information" table entirely, so nothing pointed an agent at it. Added that entry. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
…ings.json While checking fulfillment-service's broader .claude/ surface for conflicts with the cli-ux.md AGENTS.md pointer, found both Claude Code hooks in .claude/settings.json invoke bare `buf lint` -- the same staleness bug CodeRabbit caught in docs (dd93a7c), except here it's an executable automation that actually breaks: $ buf lint Failure: plugin "bin/buf-plugin-osac-lint" failed: fork/exec bin/buf-plugin-osac-lint: no such file or directory buf.yaml's OSAC_OBJECT_SHAPE lint rule requires bin/buf-plugin-osac-lint, which only `uv run dev.py lint proto` builds first. Both hooks (auto-lint after editing a .proto file, and the git-commit/gh-pr-create pre-flight gate) would fail on a fresh checkout, or any time the plugin binary is stale/absent. Swapped both to `uv run dev.py lint proto`, matching AGENTS.md, the pre-commit config, and every other correct reference to this command. Verified: `uv run dev.py lint proto` now exits 0. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
…son PR-create gate While actually simulating the settings.json hooks end-to-end (piping realistic Claude Code stdin payloads through the exact extracted command strings, per review feedback -- the prior commit had only tested the underlying `uv run dev.py lint proto` command in isolation, not the hook script itself), found the gh-pr-create branch's git-diff empty-check was broken independently of the buf-lint issue: test -z \"$(git diff --name-only)\" decodes (after JSON unescaping) to a literal backslash+quote around the substitution, i.e. `test -z '""'` at runtime -- a 2-character string, never empty, so this check always fails regardless of whether gofmt actually changed anything: $ bash -x -c 'test -z \"$(git diff --name-only)\"' + test -z '""' (exit 1, always, even on a clean tree) Fixed to single-escaped `\"..\"`, matching every other quoted variable reference in this same file. Verified end-to-end by extracting the live command from settings.json and piping realistic PostToolUse/PreToolUse JSON payloads through it: .proto edit -> lint+generate (exit 0), go.mod edit -> go mod tidy (exit 0), unrelated file/command -> no-op (exit 0), git-commit command -> lint proto (exit 0), gh-pr-create command on a clean tree -> gate passes and proceeds to ginkgo (verified via bash -x trace reaching that point, not run to completion given repo-wide test runtime). Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
AGENTS.md's quick-reference block, Linting and Code Generation section, and two proto-change reminders still told readers to run bare `buf lint`, which fails without first building bin/buf-plugin-osac-lint (the same staleness bug already fixed in .claude/settings.json's hooks and the now-deleted protobuf-conventions.md). The Automated Hooks section already correctly referenced `uv run dev.py lint proto`, so these were an internal inconsistency within the same file. Point all four at the working command. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Tommy Hughes <tohughes@redhat.com>
osac-metering/LICENSE was byte-identical (sha256 match) to the repo root LICENSE. A single root LICENSE already covers the whole osac mono-repo; this first-party subdirectory doesn't need its own copy.
|
Important Review skippedToo many files! This PR contains 2531 files, which is 2431 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (469)
📒 Files selected for processing (2531)
You can disable this status message by setting the |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if args.tls: | ||
| ctx = ssl.SSLContext(ssl.PROTOCOL_TLS_SERVER) | ||
| ctx.load_cert_chain(certfile=args.cert, keyfile=args.key) | ||
| server.socket = ctx.wrap_socket(server.socket, server_side=True) |
| 'Name' tag, or a CIDR such as 10.0.0.0/8. | ||
| """ | ||
| CIDR_RE = re.compile(r"^(\d{1,3}\.){3}\d{1,3}/\d{1,2}$") | ||
| SUBNET_RE = re.compile(r"^subnet-[A-z0-9]+$") |
This branch was successfully deployed
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
20 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
osac-metering/LICENSEwas byte-identical (sha256 match) to the repo rootLICENSE. A single root LICENSE already covers the whole mono-repo per normal convention, so this copy is redundant.Scope note
This started as a broader LICENSE-redundancy audit across the OSAC mono-repo consolidation workspace. Most of the originally-suspected duplicate LICENSE files (osac-installer, fulfillment-service, osac-operator, osac-aap top-level, docs, enhancement-proposals, osac-test-infra, graphify, enclave, bare-metal-fulfillment-operator) turned out to belong to separate, independently-versioned sibling git repositories in the parent workspace folder, not to this mono-repo's own tracked tree — so they're out of scope for this change and were left untouched.
Within this repo's actual tracked tree, only two LICENSE files were byte-identical duplicates of the root:
osac-metering/LICENSE— removed here.osac-aap/collections/ansible_collections/osac/templates/LICENSE— left in place, flagged for a human decision (see PR comment / conversation) since it's the sole LICENSE among otherwise-identical independently-packageable Ansible Galaxy collections, and it's ambiguous whether that's a deliberate per-collection distribution convention or stray leftover.The 6 vendored third-party LICENSE files under
osac-aap/vendor/ansible_collections/**were verified (diff'd) to carry genuinely different upstream licenses (GPLv3 / Apache-2.0) and are correctly untouched.Test plan
diffandsha256sumthatosac-metering/LICENSEwas byte-identical to rootLICENSEbefore removal.