skill: observed topology ≠ current traffic, native verification, PagerDuty incident → service → workload - #30
Conversation
…uty incident recipe Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6392c093c1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| OPTIONAL MATCH (i:PAGERDUTY_INCIDENT)-[:AFFECTS]->(s) | ||
| WHERE i.status IN ['triggered', 'acknowledged'] | ||
| OPTIONAL MATCH (a:PAGERDUTY_ALERT)-[:TRIGGERED]->(i) |
There was a problem hiding this comment.
When PagerDuty incidents or alerts have been deleted from the current graph, their relationships remain for history, so these matches can resurrect an incident whose stored status is still triggered/acknowledged and count deleted alerts. This can produce false candidates—or let stale rows consume the 50-row limit—for the workload-to-open-incident lookup. Add :ALIVE to both PAGERDUTY_INCIDENT and PAGERDUTY_ALERT, consistent with the recipe's current-state invariant.
Useful? React with 👍 / 👎.
| - An edge is an observation with an age (`observedAt`), not live traffic: report it as | ||
| "last observed at T", and confirm any present-tense claim (current backends, current | ||
| callers, current on-call, what an incident concerns) with a native source when one is | ||
| mounted (`kubectl`, cloud CLI, APM, PagerDuty). |
There was a problem hiding this comment.
For structural Kubernetes relationships such as EXPOSES and CONTROLS, the skill explicitly says there is no observedAt; nevertheless, this reporting rule applies the field and “last observed at T” wording to every edge. An agent following this reference therefore cannot report those relationships as instructed and may incorrectly describe declared topology as aged traffic evidence. Scope this rule to observed APM edges and give structural edges separate declared-state wording.
Useful? React with 👍 / 👎.
…APM edges (codex) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb758c20d0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| RETURN s.name AS service, i.title AS incident, i.status AS status, i.urgency AS urgency, | ||
| count(DISTINCT a) AS alerts |
There was a problem hiding this comment.
When a PagerDuty service has multiple open incidents with the same title, status, and urgency, this aggregation groups those distinct incident nodes into one row and pools their alerts in count(DISTINCT a). PagerDuty titles are not unique—as the recipe itself notes—so return i.hashedID as a grouping/output field to avoid hiding concurrent or recurring incidents.
Useful? React with 👍 / 👎.
| MATCH (i:RESOURCE {hashedID: '<incident hashedID>'})-[:AFFECTS]->(s:PAGERDUTY_SERVICE:ALIVE) | ||
| OPTIONAL MATCH (s)-[:RESOLVES_TO]->(w:ALIVE) | ||
| RETURN i.title AS incident, i.status AS status, s.name AS service, | ||
| collect(DISTINCT coalesce(w.namespace + '/', '') + coalesce(w.name, w.hashedID)) AS workloads |
There was a problem hiding this comment.
When a PagerDuty service resolves to workloads with the same namespace/name in different clusters, collect(DISTINCT ...) collapses them into one string, so the result understates the affected workloads and cannot identify which cluster is involved. Include each workload's clusterID or hashedID in the collected value rather than deduplicating solely on namespace and name.
Useful? React with 👍 / 👎.
…pe (codex) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac8d9785e0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| MATCH (i:RESOURCE {hashedID: '<incident hashedID>'})-[:AFFECTS]->(s:PAGERDUTY_SERVICE:ALIVE) | ||
| OPTIONAL MATCH (s)-[:RESOLVES_TO]->(w:ALIVE) | ||
| RETURN i.title AS incident, i.status AS status, s.name AS service, | ||
| collect(DISTINCT {cluster: w.clusterID, workload: coalesce(w.namespace + '/', '') + coalesce(w.name, w.hashedID), hashedID: w.hashedID}) AS workloads |
There was a problem hiding this comment.
When the PagerDuty service has no RESOLVES_TO edge, the optional match leaves w null, but the map literal itself is still non-null, so collect() returns a singleton map whose fields are null rather than the empty workloads list described below. An agent following the caveat may therefore fail to recognize an unmapped service; make the collected expression null when w IS NULL so collect() omits it.
Useful? React with 👍 / 👎.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 participants
Three guidance gaps surfaced by the MCP comparison thread (bugs 2–4; bug 1, the stale
EXPOSESedge, is a graph fix in commons #346 + injector + backend):max_age_hours/observedAtgate any present-tense claim; structural k8s edges are declared state,EXPOSES {ready}is readiness at the last EndpointSlice shipped. Also documents that edge props are a JSON string in Cypher (props_json), which is whyr.readyreads as null.kubectl, cloud CLI, APM, PagerDuty) for "right now"; cite which statement rests on which.PAGERDUTY_SERVICE -[:RESOLVES_TO]->workload instead of picking an incident by title; both directions, unmapped-service caveat.get_relatedstep in query-patterns now mentionsmax_age_hours. Existing package-validation assertions untouched.🤖 Generated with Claude Code