Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
✅ All CI checks and tests passed. 🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 10c4f9f | Docs | View more details | Give us feedback! |
BenchmarksBenchmark execution time: 2026-09-28 22:20:23 Comparing candidate commit 10c4f9f in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 340 metrics, 1 unstable metrics, 1 flaky benchmarks without significant changes.
|
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10c4f9fa92
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| // 2. If there is an error and the loading is ok: log as an informative error where appsec can be used | ||
| log.Error("appsec: non-critical error while loading libddwaf: %s", err.Error()) | ||
| log.Error("appsec: non-critical error while loading libddwaf: %s", err.Error()) //errtrack:ignore host native-library compatibility failure |
There was a problem hiding this comment.
Add the required AppSec README update
This change introduces persistent Error Tracking eligibility classifications throughout internal/appsec, but does not update internal/appsec/README.md; the scoped repository instructions explicitly require that README to be updated whenever AppSec changes are introduced and direct reviewers to flag omissions. Document the classification convention and rationale so future audits do not depend solely on scattered inline markers.
AGENTS.md reference: internal/appsec/AGENTS.md:L3-L7
Useful? React with 👍 / 👎.
|
|
||
| if !op.limiter.Allow() { | ||
| log.Error("appsec: too many WAF events, stopping further reporting") | ||
| log.Error("appsec: too many WAF events, stopping further reporting") //errtrack:ignore expected per-request capacity limit |
There was a problem hiding this comment.
Describe this as a global rate limit
This exclusion records the wrong rationale: op.limiter is the single limiter owned by the WAF feature, constructed from cfg.TraceRateLimit, and installed into every request context, so exhaustion here is global across requests. Only the separate maxWAFEventsPerRequest branch below is a per-request capacity limit; describe this site as expected global rate limiting on the request path so the audit record accurately explains why it is excluded.
Useful? React with 👍 / 👎.
| // to all config statuses since we can't know which config is the faulty one | ||
| if err := a.SwapRootOperation(); err != nil { | ||
| log.Error("appsec: remote config: could not apply the new security rules: %s", err.Error()) | ||
| log.Error("appsec: remote config: could not apply the new security rules: %s", err.Error()) //errtrack:ignore failure is returned through Remote Config apply status |
There was a problem hiding this comment.
Do not exclude failures that produce no apply status
This failure is not always returned through a Remote Config apply status. onRCRulesUpdate creates status entries only for ASM rule/data products, but the global callback also receives products such as ASM_FEATURES and explicitly ignored non-ASM products; it still calls SwapRootOperation afterward. When a poll contains only those ignored products, statuses is empty, so this error is merely logged and the loop cannot associate it with any apply status, making the new blanket exclusion hide a swallowed poll-path SDK failure from the Error Tracking audit.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Status
Classify 17 of the 19 AppSec audit sites as ineligible for Error Tracking reporting:
Two actionable internal startup sites remain intentionally unclassified:
Both can execute before the telemetry reporter is ready. Adoption remains gated on the startup-ordering work tracked with #5250. This PR remains a draft until that ordering is safe; it does not hide these two sites with exclusion directives.
This branch starts from
mainand is not stacked on another migration PR.Validation
go test ./appsec ./internal/appsec/... ./instrumentation/appsec/... -count=1go vet ./appsec ./internal/appsec/... ./instrumentation/appsec/...make lint/errloggit diff --check