HYPERFLEET-537 - docs: configurable conditions-based decision making#102
HYPERFLEET-537 - docs: configurable conditions-based decision making#102
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces fixed max-age timeouts with a configurable CEL-based message_decision block that defines named params (ref_time, is_ready, age_exceeded_ready, age_exceeded_not_ready) and a boolean result expression to decide publish vs skip. Adds immediate reconciliation when resource.generation > condition(resource, "Available").observed_generation by publishing a CreateEvent without waiting for age logic. CEL expressions are compiled at startup (fail-fast). Sentinel configs (including regional variants) now include message_decision; broker/config separation introduced; metrics and skip-reason labels updated to reflect generation_mismatch and message_decision outcomes. Sequence Diagram(s)sequenceDiagram
participant Config as Configuration
participant CELEngine as CEL Engine
participant Monitor as Resource Monitor
participant Decision as Decision Loop
participant Broker as Event Broker
Config->>CELEngine: Load & compile message_decision (fail-fast)
CELEngine-->>Config: Compilation OK
Monitor->>Decision: Poll resource / read conditions
Decision->>Decision: Check resource.generation vs condition(...).observed_generation
alt generation > observed_generation
Decision->>Broker: Publish CreateEvent (immediate)
else generation == observed_generation
Decision->>CELEngine: Evaluate message_decision params (ref_time,is_ready,age_exceeded_*)
CELEngine-->>Decision: Boolean result
alt result == true
Decision->>Broker: Publish Reconciliation Event
else result == false
Decision-->>Decision: Skip (record reason: message_decision)
end
end
Broker-->>Broker: Queue/forward event
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)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
hyperfleet/components/sentinel/sentinel.md (1)
608-638:⚠️ Potential issue | 🟠 MajorUse one
observed_generationpath throughout the spec.This section compares against
resource.status.conditions.Ready.observed_generation, but the rest of the page and the test scenarios useresource.status.observed_generation. Please normalize on a single contract here; otherwise the Decision Engine description is ambiguous.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet/components/sentinel/sentinel.md` around lines 608 - 638, The document inconsistently references two observed_generation paths; normalize to a single contract by replacing the use of resource.status.conditions.Ready.observed_generation with the canonical resource.status.observed_generation (or vice versa if you prefer that canonical name) everywhere in the Decision Logic and Implementation Requirements so the generation check in Evaluate(resource, now) consistently compares resource.generation against the same observed_generation path; update all references, examples, and test scenario mentions and ensure the text and logging (e.g., the generation mismatch reason string) reflect the chosen single field name.hyperfleet/components/sentinel/sentinel-deployment.md (1)
43-61:⚠️ Potential issue | 🟠 MajorThe pod spec still doesn't consume the documented broker ConfigMap.
This file now introduces
hyperfleet-sentinel-broker, but the Deployment only pullsGCP_PROJECT_IDfromsentinel-configand never imports anyBROKER_*keys. As written, the RabbitMQ example never reaches the container environment, and Pub/Sub has two competing config sources.Also applies to: 121-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet/components/sentinel/sentinel-deployment.md` around lines 43 - 61, The Deployment pod spec is missing env entries that consume the new hyperfleet-sentinel-broker ConfigMap so BROKER_* values never reach the container; update the env block to add the required BROKER_* env vars (e.g., BROKER_TYPE, BROKER_URL, BROKER_USER, BROKER_PASSWORD or whatever keys your broker consumer expects) with valueFrom: configMapKeyRef name: hyperfleet-sentinel-broker and the appropriate key names, ensure BROKER_CREDENTIALS (if still needed) remains sourced from sentinel-secrets (secretKeyRef) only when required, and remove or reconcile any duplicate BROKER sources so Pub/Sub continues to use GCP_PROJECT_ID from sentinel-config while RabbitMQ/other brokers use hyperfleet-sentinel-broker.
🧹 Nitpick comments (1)
hyperfleet/components/sentinel/sentinel-deployment.md (1)
126-134: Expose rule-oriented labels in the metrics spec.Once skip/publish decisions come from arbitrary CEL rules, a fixed
ready_state={ready,not_ready}label no longer tells operators which rule matched and cannot represent the no-match fallback. Amatched_ruleordecision_reasonlabel would fit the new configuration model much better.Also applies to: 209-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet/components/sentinel/sentinel-deployment.md` around lines 126 - 134, The metrics spec currently emits a fixed ready_state label which can't convey which CEL rule matched or represent a no-match fallback; update the metrics emission to include a rule-oriented label (e.g., matched_rule or decision_reason) populated with the matching rule name (use the rule "name" values such as isReady and isNotReady) and ensure a deterministic fallback value (e.g., "no_match") when no rule matches; apply the same change to the other rules block referenced (lines ~209-225) so all rule-based decisions expose the matching rule name and a clear no-match reason in metrics.
🤖 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/architecture-summary.md`:
- Around line 268-279: Update the later sequence diagrams so their decision
boxes match the documented Decision Algorithm: change the checks to first
compare resource.generation vs resource.status.observed_generation
(generation-first reconciliation), then evaluate CEL condition rules to select
matched_rule.max_age (or smallest max_age if none), compute reference_time from
the rule expression, and use now >= reference_time + max_age to trigger
PublishEvent(CreateEvent(resource)); replace any hardcoded phase != "Ready" or
last_updated_time + 10s logic with these same symbols (FetchResources,
resource.generation, resource.status.observed_generation, matched_rule.max_age,
reference_time, PublishEvent, CreateEvent, CEL) so the diagrams and prose are
consistent.
In `@hyperfleet/components/sentinel/sentinel.md`:
- Around line 238-255: Update the doc to specify the timestamp fallback used
when no rule matches: state that reference_time (used by nextEventTime) will use
conditionTime(resource, "<conditionName>") if available, otherwise fall back to
resource.metadata.creationTimestamp (and if that is missing, use the resource's
observedGeneration/now as a last resort); mention this behavior wherever
reference_time, nextEventTime, or conditionTime are described (including the
default-rule/no-match section) so Test 9’s scenario is fully specified. Ensure
the prose notes that rules still pick the smallest max_age when none match but
that the timestamp for ROI calculation falls back as described.
---
Outside diff comments:
In `@hyperfleet/components/sentinel/sentinel-deployment.md`:
- Around line 43-61: The Deployment pod spec is missing env entries that consume
the new hyperfleet-sentinel-broker ConfigMap so BROKER_* values never reach the
container; update the env block to add the required BROKER_* env vars (e.g.,
BROKER_TYPE, BROKER_URL, BROKER_USER, BROKER_PASSWORD or whatever keys your
broker consumer expects) with valueFrom: configMapKeyRef name:
hyperfleet-sentinel-broker and the appropriate key names, ensure
BROKER_CREDENTIALS (if still needed) remains sourced from sentinel-secrets
(secretKeyRef) only when required, and remove or reconcile any duplicate BROKER
sources so Pub/Sub continues to use GCP_PROJECT_ID from sentinel-config while
RabbitMQ/other brokers use hyperfleet-sentinel-broker.
In `@hyperfleet/components/sentinel/sentinel.md`:
- Around line 608-638: The document inconsistently references two
observed_generation paths; normalize to a single contract by replacing the use
of resource.status.conditions.Ready.observed_generation with the canonical
resource.status.observed_generation (or vice versa if you prefer that canonical
name) everywhere in the Decision Logic and Implementation Requirements so the
generation check in Evaluate(resource, now) consistently compares
resource.generation against the same observed_generation path; update all
references, examples, and test scenario mentions and ensure the text and logging
(e.g., the generation mismatch reason string) reflect the chosen single field
name.
---
Nitpick comments:
In `@hyperfleet/components/sentinel/sentinel-deployment.md`:
- Around line 126-134: The metrics spec currently emits a fixed ready_state
label which can't convey which CEL rule matched or represent a no-match
fallback; update the metrics emission to include a rule-oriented label (e.g.,
matched_rule or decision_reason) populated with the matching rule name (use the
rule "name" values such as isReady and isNotReady) and ensure a deterministic
fallback value (e.g., "no_match") when no rule matches; apply the same change to
the other rules block referenced (lines ~209-225) so all rule-based decisions
expose the matching rule name and a clear no-match reason in metrics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d3a74e28-76b6-4274-99be-601c3ab31699
📒 Files selected for processing (4)
hyperfleet/architecture/architecture-summary.mdhyperfleet/components/sentinel/sentinel-config.yamlhyperfleet/components/sentinel/sentinel-deployment.mdhyperfleet/components/sentinel/sentinel.md
There was a problem hiding this comment.
🧹 Nitpick comments (3)
hyperfleet/components/sentinel/sentinel.md (1)
905-916: Test 9 could be clearer about reference_time fallback behavior.Test 9 documents the scenario where no condition rule matches, correctly stating it falls back to the smallest max_age. However, the test states
reference_time = now() - 15swithout explaining how this value is determined when the resource "has no Ready condition (or unexpected status)."According to line 249 in this file, when the reference_time expression evaluation fails, it should fall back to
created_time. Test 9 should clarify this to fully document the edge case behavior mentioned in the past review.🧪 Suggested test clarification
**Test 9: No condition rule matches → use smallest max_age as fallback**Given:
- Resource has no Ready condition (or unexpected status)
- resource.generation = 1
- resource.status.observed_generation = 1
- reference_time = now() - 15s
- reference_time = resource.metadata.created_time = now() - 15s # Falls back to created_time when conditionTime() fails
- No rule matches → fallback to smallest max_age (10s)
Then:- Decision: PUBLISH
- Reason: "max age expired (fallback: no rule matched)"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet/components/sentinel/sentinel.md` around lines 905 - 916, Update Test 9 in sentinel.md to explicitly state that when the resource "has no Ready condition (or unexpected status)" the reference_time expression evaluation fails and therefore reference_time falls back to resource.metadata.created_time (e.g., "reference_time = resource.metadata.created_time = now() - 15s # Falls back to created_time when conditionTime() fails"); keep the rest of the scenario and expected Decision/Reason unchanged so readers understand the fallback behavior described around line 249.hyperfleet/architecture/architecture-summary.md (2)
268-279: Clarify reference_time behavior when no rule matches.The decision algorithm mentions using "smallest max_age if no match" (line 275) but doesn't specify what
reference_timevalue is used in that scenario. Sincereference_timeis typically derived from a CEL expression likeconditionTime(resource, "Ready"), readers may wonder what timestamp applies when no rule matches (e.g., when the resource lacks the expected condition).Based on the past review comment indicating this was addressed in sentinel.md, consider adding a brief note here for completeness.
📝 Suggested addition
Evaluate condition rules (CEL expressions) to find matching rule max_age = matched_rule.max_age (or smallest max_age if no match) - reference_time = evaluate reference_time expression + reference_time = evaluate reference_time expression (falls back to created_time if evaluation fails) IF now >= reference_time + max_age: PublishEvent(broker, CreateEvent(resource))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet/architecture/architecture-summary.md` around lines 268 - 279, The Decision Algorithm text is unclear about which reference_time to use when no rule matches; update the description for reference_time (the symbol reference_time) to state the fallback behavior: when matched_rule is absent, evaluate reference_time using the same default expression rules (e.g., evaluate the CEL expression if provided, otherwise use resource.status.observed_generation timestamp or resource.metadata.creationTimestamp as a final fallback) and document that max_age uses the smallest configured value when no match; mention the relevant symbols FetchResources, matched_rule, max_age, reference_time, PublishEvent and CreateEvent so readers can find and confirm the logic.
232-240: Document CEL custom functions or cross-reference their definitions.The configuration introduces CEL expressions using custom functions like
conditionTime(resource, "Ready")andstatus(resource, "Ready"), but this file doesn't explain what these functions do or their return types. Readers encountering this architecture summary may struggle to understand the decision logic without knowing the function semantics.📚 Suggested clarification
Add a brief note after the configuration example explaining the custom CEL functions:
conditions: reference_time: 'conditionTime(resource, "Ready")' rules: - name: isReady expression: 'status(resource, "Ready") == "True"' max_age: 30m - name: isNotReady expression: 'status(resource, "Ready") == "False"' max_age: 10s + +# CEL Custom Functions: +# - conditionTime(resource, "conditionName"): Returns the last_updated_time of the specified condition +# - status(resource, "conditionName"): Returns the status value ("True", "False", "Unknown") of the specified condition +# See sentinel.md for complete CEL function documentation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet/architecture/architecture-summary.md` around lines 232 - 240, Add a short explanatory note after the conditions example that documents the custom CEL functions conditionTime(resource, "Ready") and status(resource, "Ready"): state what each function returns (e.g., conditionTime returns an RFC3339 timestamp or duration since the named condition was set; status returns the string value of the named condition such as "True"/"False"/"Unknown"), describe expected parameter types (resource object and condition name string), and either link or cross-reference the function implementations/definitions so readers can find the exact semantics and return types used by the CEL expressions in this file.
🤖 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/architecture/architecture-summary.md`:
- Around line 268-279: The Decision Algorithm text is unclear about which
reference_time to use when no rule matches; update the description for
reference_time (the symbol reference_time) to state the fallback behavior: when
matched_rule is absent, evaluate reference_time using the same default
expression rules (e.g., evaluate the CEL expression if provided, otherwise use
resource.status.observed_generation timestamp or
resource.metadata.creationTimestamp as a final fallback) and document that
max_age uses the smallest configured value when no match; mention the relevant
symbols FetchResources, matched_rule, max_age, reference_time, PublishEvent and
CreateEvent so readers can find and confirm the logic.
- Around line 232-240: Add a short explanatory note after the conditions example
that documents the custom CEL functions conditionTime(resource, "Ready") and
status(resource, "Ready"): state what each function returns (e.g., conditionTime
returns an RFC3339 timestamp or duration since the named condition was set;
status returns the string value of the named condition such as
"True"/"False"/"Unknown"), describe expected parameter types (resource object
and condition name string), and either link or cross-reference the function
implementations/definitions so readers can find the exact semantics and return
types used by the CEL expressions in this file.
In `@hyperfleet/components/sentinel/sentinel.md`:
- Around line 905-916: Update Test 9 in sentinel.md to explicitly state that
when the resource "has no Ready condition (or unexpected status)" the
reference_time expression evaluation fails and therefore reference_time falls
back to resource.metadata.created_time (e.g., "reference_time =
resource.metadata.created_time = now() - 15s # Falls back to created_time when
conditionTime() fails"); keep the rest of the scenario and expected
Decision/Reason unchanged so readers understand the fallback behavior described
around line 249.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6df6ec8f-eafa-48ee-a059-4c1608fcea0d
📒 Files selected for processing (3)
hyperfleet/architecture/architecture-summary.mdhyperfleet/components/sentinel/sentinel-deployment.mdhyperfleet/components/sentinel/sentinel.md
🚧 Files skipped from review as they are similar to previous changes (1)
- hyperfleet/components/sentinel/sentinel-deployment.md
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/architecture-summary.md`:
- Around line 274-279: Update the summary algorithm to specify the same fallback
used in sentinel.md for computing reference_time: when evaluating
conditionTime(resource, "Ready") returns no timestamp or no rule matches (so
matched_rule is absent), fall back to resource.created_time (or the documented
alternative in sentinel.md) before computing reference_time + max_age;
explicitly mention reference_time, matched_rule, max_age, and
conditionTime(resource, "Ready") in the text so the no-match and no-timestamp
paths produce a well-defined reference_time used in the IF now >= reference_time
+ max_age check.
- Around line 218-222: Update the responsibilities list to reflect the new
generation-first decision model: replace references to the old trigger-based
checks (status.phase and status.last_updated_time) with the new reconciliation
decision flow that uses generation-first reconciliation plus CEL-selected
max_age and reference_time; specifically, modify the Decision Logic bullet to
state that reconciliation is triggered by new generation detection and/or
CEL-evaluated max_age/reference_time rules (using CEL expressions) and remove or
deprecate the prior reliance on status.phase/status.last_updated_time so the
summary uses a single, consistent decision model.
In `@hyperfleet/components/sentinel/sentinel.md`:
- Around line 215-218: State in the main algorithm that when evaluating
condition(resource, "Available").observed_generation it must default to 0 if the
Available condition is missing (to avoid CEL/runtime errors), and treat
resource.generation > observed_generation using that default; update the rule
that references resource.generation and condition(resource,
"Available").observed_generation to explicitly say "missing Available condition
= 0 (observed_generation = 0)" so new resources are handled deterministically
(this mirrors the behavior described later in Test 8).
- Around line 24-26: Clarify ownership of timing/generation fields by explicitly
stating whether adapters write per-condition fields
(status.conditions[].last_updated_time and
status.conditions[].observed_generation) or only the top-level observed_time /
observed_generation and the API derives the per-condition values; update the
sentinel.md text to pick one model (recommended: adapters POST top-level
observed_time/observed_generation and the API populates status.conditions[]
last_updated_time and observed_generation) and change the lines describing "Uses
adapter status updates" and any subsequent contract wording to reflect that
single, unambiguous flow, referencing the symbols
status.conditions[].last_updated_time, status.conditions[].observed_generation,
observed_time, and observed_generation so readers know which side is
authoritative.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cc00851b-dcd1-4c20-952a-ecaf7ebcb1d1
📒 Files selected for processing (2)
hyperfleet/architecture/architecture-summary.mdhyperfleet/components/sentinel/sentinel.md
rafabene
force-pushed
the
HYPERFLEET-537
branch
from
cd8f580 to
fcd347c
Compare
rafabene
force-pushed
the
HYPERFLEET-537
branch
from
fcd347c to
4ac6092
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/components/sentinel/sentinel.md`:
- Around line 215-226: Add explicit wording to the Decision Logic section
clarifying missing-Available behavior: state that when condition(resource,
"Available") is absent its observed_generation should be treated as 0 (so
condition(resource, "Available").observed_generation defaults to 0), which
causes the Generation Mismatch check to trigger immediate reconciliation for new
resources; also reference that this aligns with message_decision.params and
message_decision.result evaluation flow so readers understand the precedence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4d00326d-9dd5-46bc-a61e-b8141e6798f0
📒 Files selected for processing (5)
hyperfleet/architecture/architecture-summary.mdhyperfleet/components/adapter/framework/adapter-flow-diagrams.mdhyperfleet/components/sentinel/sentinel-config.yamlhyperfleet/components/sentinel/sentinel-deployment.mdhyperfleet/components/sentinel/sentinel.md
🚧 Files skipped from review as they are similar to previous changes (1)
- hyperfleet/components/sentinel/sentinel-config.yaml
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
hyperfleet/components/sentinel/sentinel-deployment.md (2)
215-226:⚠️ Potential issue | 🟠 Major
reasonlabel semantics are internally inconsistent.Line 215 describes skipped reasons as
"matched rule name"/"no_match", while Line 226 describes decision reasons like"generation_mismatch"/"message_decision"and param-specific skip reasons. Please define one canonical taxonomy to avoid divergent metric implementations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet/components/sentinel/sentinel-deployment.md` around lines 215 - 226, Unify the `reason` label taxonomy across the sentinel metrics: choose and document one canonical set of reason values (e.g., "no_match", "rule:<rule_name>", "generation_mismatch", "message_decision", "param:<param_name>") and update the metric docs for `hyperfleet_sentinel_resources_skipped_total` and any decision-related descriptions so they reference this single taxonomy; ensure the `reason` semantics are explicitly described (how to encode rule names vs param-specific skips) and referenced in the Implementation Requirements so all code paths updating `reason` (the skipped counter and decision recording) use the same canonical values.
60-65:⚠️ Potential issue | 🟠 Major
BROKER_CREDENTIALSis still required in the manifest despite being documented as optional.Line 60 says credentials are only needed for some brokers, but Lines 61-65 always require
sentinel-secrets/broker-credentials. This can block startup when that key is intentionally absent.Suggested doc fix
- name: BROKER_CREDENTIALS valueFrom: secretKeyRef: name: sentinel-secrets key: broker-credentials + optional: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet/components/sentinel/sentinel-deployment.md` around lines 60 - 65, The manifest currently requires BROKER_CREDENTIALS unconditionally (env var BROKER_CREDENTIALS using secretKeyRef sentinel-secrets → broker-credentials); make this optional by either removing/commenting the hard requirement in the example and documenting it as optional, or update the secret reference to allow absence by setting the SecretKeySelector to optional (i.e., mark the secretKeyRef for broker-credentials as optional) so the pod can start when sentinel-secrets/broker-credentials is intentionally absent; update the text around BROKER_CREDENTIALS to reflect the optional behavior.
♻️ Duplicate comments (2)
hyperfleet/components/sentinel/sentinel.md (1)
24-25:⚠️ Potential issue | 🟠 MajorField ownership is misstated for condition-level status data.
Line 24/Line 42 currently say adapters update
status.conditions[].last_updated_timeandstatus.conditions[].observed_generationdirectly. That conflicts with the documented contract where adapters POST status reports and the API produces aggregated condition fields Sentinel reads. Please make ownership explicit to avoid implementation drift.Also applies to: 42-42
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet/components/sentinel/sentinel.md` around lines 24 - 25, The doc incorrectly states that adapters update status.conditions[].last_updated_time and status.conditions[].observed_generation directly; instead, state that adapters submit status reports (e.g., via POST) and the API aggregates those reports into the condition-level fields that Sentinel reads; update the sentences mentioning status.conditions[].last_updated_time and status.conditions[].observed_generation (and any text about adapters updating them) to clarify that adapters produce reports and the API owns and sets the aggregated condition fields which Sentinel consumes.hyperfleet/architecture/architecture-summary.md (1)
219-220:⚠️ Potential issue | 🟠 MajorUse the concrete observed-generation source, not ambiguous
observedGeneration.At Line 219 and Line 268,
observedGenerationis undefined/ambiguous. This should explicitly referencecondition(resource, "Available").observed_generation(with the documented default behavior when missing) so the algorithm matches the API contract and the sentinel docs.Also applies to: 268-273
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hyperfleet/architecture/architecture-summary.md` around lines 219 - 220, The docs reference an ambiguous observedGeneration; update the text and any pseudo-code that uses resource.generation > observedGeneration to instead use the concrete API field condition(resource, "Available").observed_generation (and mention the documented default behavior when that field is missing), and apply this change to both places that mention observedGeneration (the generation check and the later algorithm block that spans the configurable message decision), so the algorithm explicitly matches the API contract and sentinel docs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@hyperfleet/components/sentinel/sentinel-deployment.md`:
- Around line 215-226: Unify the `reason` label taxonomy across the sentinel
metrics: choose and document one canonical set of reason values (e.g.,
"no_match", "rule:<rule_name>", "generation_mismatch", "message_decision",
"param:<param_name>") and update the metric docs for
`hyperfleet_sentinel_resources_skipped_total` and any decision-related
descriptions so they reference this single taxonomy; ensure the `reason`
semantics are explicitly described (how to encode rule names vs param-specific
skips) and referenced in the Implementation Requirements so all code paths
updating `reason` (the skipped counter and decision recording) use the same
canonical values.
- Around line 60-65: The manifest currently requires BROKER_CREDENTIALS
unconditionally (env var BROKER_CREDENTIALS using secretKeyRef sentinel-secrets
→ broker-credentials); make this optional by either removing/commenting the hard
requirement in the example and documenting it as optional, or update the secret
reference to allow absence by setting the SecretKeySelector to optional (i.e.,
mark the secretKeyRef for broker-credentials as optional) so the pod can start
when sentinel-secrets/broker-credentials is intentionally absent; update the
text around BROKER_CREDENTIALS to reflect the optional behavior.
---
Duplicate comments:
In `@hyperfleet/architecture/architecture-summary.md`:
- Around line 219-220: The docs reference an ambiguous observedGeneration;
update the text and any pseudo-code that uses resource.generation >
observedGeneration to instead use the concrete API field condition(resource,
"Available").observed_generation (and mention the documented default behavior
when that field is missing), and apply this change to both places that mention
observedGeneration (the generation check and the later algorithm block that
spans the configurable message decision), so the algorithm explicitly matches
the API contract and sentinel docs.
In `@hyperfleet/components/sentinel/sentinel.md`:
- Around line 24-25: The doc incorrectly states that adapters update
status.conditions[].last_updated_time and
status.conditions[].observed_generation directly; instead, state that adapters
submit status reports (e.g., via POST) and the API aggregates those reports into
the condition-level fields that Sentinel reads; update the sentences mentioning
status.conditions[].last_updated_time and
status.conditions[].observed_generation (and any text about adapters updating
them) to clarify that adapters produce reports and the API owns and sets the
aggregated condition fields which Sentinel consumes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4d5f6306-2133-46cc-ade8-1d3641a17a32
📒 Files selected for processing (5)
hyperfleet/architecture/architecture-summary.mdhyperfleet/components/adapter/framework/adapter-flow-diagrams.mdhyperfleet/components/sentinel/sentinel-config.yamlhyperfleet/components/sentinel/sentinel-deployment.mdhyperfleet/components/sentinel/sentinel.md
🚧 Files skipped from review as they are similar to previous changes (2)
- hyperfleet/components/adapter/framework/adapter-flow-diagrams.md
- hyperfleet/components/sentinel/sentinel-config.yaml
There was a problem hiding this comment.
Focusing on technical gaps and consistency issues.
Overall direction is sound — making the decision engine configurable aligns with the adapter framework's CEL patterns. But the documentation needs tightening before it becomes the implementation contract.
rafabene
force-pushed
the
HYPERFLEET-537
branch
from
8cd1e40 to
1403e8a
Compare
- Introduce message_decision with CEL params and boolean result expression - Single condition(name) CEL function for accessing resource status - Pure CEL operators (no AND/OR aliases) - Unified decision logic (no hardcoded generation check) - Ready==False covers generation mismatch (API computes it) - Remove sentinel-config.yaml and sentinel-deployment.md to avoid drift - Update architecture-summary.md to match
rafabene
force-pushed
the
HYPERFLEET-537
branch
from
1403e8a to
f51819c
Compare
4 participants
Summary
max_age_not_ready/max_age_readywith configurableconditionssection using CEL expressionsarchitecture-summary.mdfor consistencyFiles Changed
sentinel-config.yamlconditionsblock replacesmax_age_not_ready/max_age_readysentinel.mdsentinel-deployment.mdarchitecture-summary.mdRelated
Test plan
max_age_not_ready/max_age_readyin the repositoryconditionsformatSummary by CodeRabbit
Documentation
New Features
Chores