Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: akash-network/console/.coderabbit.yaml Review profile: CHILL Plan: Essentials Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
bd62327 to
14c8627
Compare
ea3c697 to
fd2b01f
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked the shared evidenceWriteFailures counter that both the pre-existing evidence-write failure path and the new recordBehaviouralFindings write failure (probe-evidence.service.ts) now increment — confirmed they do conflate under one unlabeled metric, but this is an observability gap rather than a functional bug, so it doesn't add a blocking concern beyond what's already flagged inline.
Extended reasoning...
Three confirmed bugs were already surfaced as inline comments on this PR (truncated-probe rows scored as fully scanned in probe-evidence.service.ts, the sensitivity report missing from the replay service's completion log, and IPv6 relay-endpoint string-equality mismatch in network-isolation.ts), so the PR already signals that human review is needed and I should not restate those. I independently verified the one ruled-out candidate issue in the task data by reading the actual diff: recordBehaviouralFindings in probe-evidence.service.ts calls this.instrumentation.recordEvidenceWriteFailure() on failure (line ~83), which is the exact same counter (workload_abuse_evidence_write_failures_total, no labels) incremented by the original evidence-insert failure path — confirmed via the diff to workload-abuse-instrumentation.service.ts, which added a separate labeled behaviouralFindings counter for successful findings but did not add a distinct failure counter/label for the new write path. This is a real but minor observability gap (alerting on evidence-write failures can no longer distinguish which write failed), not a correctness or security bug, consistent with it being ruled out as non-blocking.
Given three confirmed, non-trivial bugs already exist as inline comments, this PR clearly needs human attention regardless of what I add, so per the guidelines I should not approve and should not duplicate the inline findings in a top-level body. The narrow exception for speaking while findings exist is to note something concrete I separately examined and ruled out, which I have here (the shared-metric conflation), stated briefly and without implying it clears the PR or that no further review is needed.
Findings marked 🟡 are optional suggestions and need no follow-up push.
There was a problem hiding this comment.
Beyond the inline findings, I also checked findCreatedBetween (workload-probe-evidence.repository.ts) for unbounded-result risk — it has no LIMIT, but it's only reachable from the new offline replay CLI over an operator-supplied window, not a request-path or untrusted-input query, so I ruled it out as a memory-exhaustion concern for this PR.
Extended reasoning...
This run's inline findings cover two confirmed bugs in BehaviouralSignalReplayService (unvalidated fixture parsing via JSON.parse(...) as BehaviouralReplayFixture, and the usesDatabase flag ignoring options.until when only that bound is supplied). Beyond those, I independently examined WorkloadProbeEvidenceRepository.findCreatedBetween for the same class of risk (unbounded query results) since it backs the same CLI tool, and ruled it out: it is invoked only by the read-only, operator-driven replay-behavioural-signals command over an explicit window, not by any API route or untrusted caller, so the blast radius of an unbounded read is limited to the operator's own tooling rather than a production or security concern.
Findings marked 🟡 are optional suggestions and need no follow-up push.
7f3f1a1 to
c9013fa
Compare
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked WorkloadProbeEvidenceRepository.findCreatedBetween (apps/api/src/workload-abuse/repositories/workload-probe-evidence/workload-probe-evidence.repository.ts:37-43) for an unbounded read — it has no .limit(), but it's only reached from the operator-run replay-behavioural-signals CLI (never a request path) and defaults to a 30-day window, so I didn't flag it as a bug.
Extended reasoning...
This run's confirmed finding (the usesDatabase fixture-load-failure fallback at behavioural-signal-replay.service.ts:66) is new and already going out as an inline comment, so it isn't restated here. I additionally traced findCreatedBetween's lack of a .limit() clause to see whether it posed the same kind of unbounded-scan risk as the confirmed issue, but concluded it doesn't warrant a finding: it's only invoked from the read-only, operator-triggered replay-behavioural-signals CLI path (not from any request handler), and the caller (#loadFromDatabase/#resolveWindow) always supplies a bounded window that defaults to 30 days when --since/--until are omitted, so this is an intentionally-open-ended offline tool rather than an unbounded production query.
c9013fa to
3894656
Compare
There was a problem hiding this comment.
I reviewed this PR again and found no new bugs. The commits pushed since my last review (bb2d2cf, fbfa6f1, 3894656) address all five previously raised findings in the code itself — not merely via self-resolved threads — plus the newly reported blank-env-var config crash. Because this is a large (1503-line) new abuse-detection subsystem with security implications, a human look is still worthwhile before merging.
What was reviewed:
- Shell-status gating:
evaluateBehaviouralSignalsnow returns no findings unlessshellStatus === "completed", fixing the truncated-probe false positive. - IPv6 relay matching:
normalizeHostnow expands compressed IPv6 to match the probe's decoded fixed-width form. - Replay fixes:
--untilalone now triggers a database query, the threshold sweep is now logged, and per-fixture JSON is now schema-validated with per-file failure isolation. - Config fix: blank
WORKLOAD_ABUSE_SIGNAL_RELAY_ENDPOINTSnow falls back to its default instead of crashing config resolution.
Extended reasoning...
Overview
This is the fifth review pass on a PR that adds a new "behavioural signals" detection layer (accel-without-artifacts + network-isolation) to apps/api's workload-abuse module, plus a replay/CLI tool for tuning thresholds against fixtures or recorded evidence. Four commits (d115551, bb2d2cf, fbfa6f1, 3894656) landed since the base commit in this diff range, with the latter three specifically responding to five inline findings from my prior reviews plus one additional finding about a blank env var crashing config resolution.
Verification of prior findings
I independently re-read the current code (not relying on the author's thread self-resolutions, which are not evidence of a fix) for each previously raised issue: (1) the shell-status/truncation gating issue in probe-evidence.service.ts is now fixed via a shared evaluateBehaviouralSignals guard on shellStatus !== COMPLETE_SHELL_STATUS, applied identically in both the live path and the replay service; (2) the IPv6 relay-endpoint mismatch is fixed via a normalizeHost function that expands compressed IPv6 notation to the kernel's fixed-width hex form before comparison; (3) the usesDatabase logic in behavioural-signal-replay.service.ts was reworked to key off whether fixtures were explicitly requested (bundledFixtures || fixturePaths?.length) rather than whether the resulting fixture list ended up non-empty, which fixes both the --until-only case and the silent-fallback-on-bad-fixture-path case (the "nit 66" item that was still open in my last review); (4) the sensitivity sweep is now included in the BEHAVIOURAL_REPLAY_COMPLETED log; (5) fixture files are now validated against a real Zod schema (FIXTURE_SCHEMA.parse) with per-file try/catch so one malformed file no longer aborts the whole run; (6) the new WORKLOAD_ABUSE_SIGNAL_RELAY_ENDPOINTS env var now uses the same blankToUndefined preprocessing as its sibling field, so a blank value falls back to "[]" instead of throwing. All six are genuine code fixes, not just resolved GitHub threads.
Security risks
The module only records/counts findings and gates everything behind a feature flag (WORKLOAD_ABUSE_BEHAVIOURAL_SIGNALS_ENABLED) — no verdict or enforcement logic is touched, matching the PR description. No SQL injection, auth-bypass, or data-exposure concerns were apparent in the read-only replay service or the repository/controller changes.
Level of scrutiny and other factors
Despite no new findings this round, this remains a large, non-trivial addition (1503 insertions across 27 files) to a security/abuse-detection subsystem, including a new CLI surface, config surface, and persistence changes. Given the size and sensitivity, and consistent with my role's guidance to prefer deferring over approving large/security-adjacent changes even when no bugs are found, I'm recommending a human still take a look rather than approving outright, while noting that the concrete, previously-flagged issues have been resolved in code.
Two shape signals now run over the evidence rows a probe writes: one for a workload that holds the accelerator while carrying nothing on disk, one for a workload with nothing established in either direction. Both are recorded on the row and counted, neither changes a verdict, and both stay off until the flag is set. A console command re-scores recorded evidence, or a corpus of fixture shapes, and reports what each agreement length would have reached. It reads and never writes.
…inished collecting A section that stopped early looks exactly like a section with nothing in it, and both signals read an absence as the thing worth noticing. Snapshots now carry the status of the shell that produced them, and a collection that was cut short supports no signal, in the live path and in the replay. The replay honours a window given by its end alone, skips a fixture file it cannot parse instead of losing the whole run with it, and logs the threshold sweep it computes so an operator running the command actually sees it. Relay addresses configured in the short form of an IPv6 address now match the expanded form the probe decodes.
The evidence shape now names connections for what they are and keeps the socket state with each one, so the signals and the replay corpus carry a half open connection as such.
…nfig A variable that is declared but left empty reached the JSON parse and turned every resolution of the workload abuse config into a validation error. It now falls back to the default the same way the sibling list does. A replay that asked for fixtures and could read none of them reports nothing rather than quietly scanning a month of recorded evidence instead.
3894656 to
b7f7eb3
Compare
There was a problem hiding this comment.
I reviewed the latest commits and the seven issues raised across my earlier rounds on this PR (truncated-shell gating, IPv6 relay matching, until-only window handling, fixture parse failures, fixture-vs-database fallback, blank RELAY_ENDPOINTS config, and the missing sensitivity log) all now have corresponding code fixes with matching new test cases — I checked each one against the current source rather than just the commit messages. No new bugs were found this round. Given the size and abuse-detection sensitivity of this change, a human look is still worthwhile before merge.
Extended reasoning...
Overview
This diff adds a "behavioural signals" scoring layer to apps/api's workload-abuse trial-probe pipeline: two pure evaluators (accelerator-without-artifacts, network-isolation) combined into a candidate conjunction, new env-driven thresholds, persistence of findings via a new repository method, an OTel counter, and a read-only CLI replay tool with bundled JSON fixtures. This is round 6 of review on this PR; three new fix commits (d5f8fa7, db2cb44, b7f7eb3) landed after the last review round.
Security risks
No injection, auth-bypass, or data-exposure concerns. The feature only records/counts findings behind a disabled-by-default flag and does not alter enforcement verdicts. The main risk surface was correctness of the detection logic itself (false positives/negatives), which is what the prior review rounds focused on.
Level of scrutiny
I verified, by reading the current source directly (not the commit messages), that all seven previously-raised issues are fixed: evaluateBehaviouralSignals now gates on shellStatus === "completed" (types.ts/evaluate-behavioural-signals.ts), normalizeHost expands compressed IPv6 for relay matching (network-isolation.ts), usesDatabase in the replay service now considers until alone and distinguishes "no fixtures requested" from "fixtures requested but all failed to load," #readFixture wraps parsing in try/catch with parseReplayFixture doing real zod validation, WORKLOAD_ABUSE_SIGNAL_RELAY_ENDPOINTS is now wrapped in blankToUndefined, and the BEHAVIOURAL_REPLAY_COMPLETED log now includes sensitivity. Each fix has a corresponding new spec (probe-evidence.service.spec.ts:131, network-isolation.spec.ts IPv6 tests, several new cases in behavioural-signal-replay.service.spec.ts, env.config.spec.ts blank-string test), so these aren't just plausible-looking patches — they're exercised.
Other factors
Despite the fixes checking out, this remains an XL, multi-file feature in a security-relevant subsystem with a track record of five prior rounds each surfacing real bugs, and I was unable to execute the test suite in this sandboxed run (Bash was denied) to confirm green CI, relying instead on static reading of source and specs. That combination — scale, sensitivity, and no executed test confirmation — keeps this above the bar for silent auto-approval even though no new issues were found this round.
1 participant
Why
Ref CON-978
Stacked on #3982, which is where the evidence rows these signals read come from. Review that one first.
String matching only catches a workload we have already seen. These two signals describe a shape instead, so the probe has something to record about a workload that matches nothing. They record and count findings, nothing else: no verdict moves, and both stay off until the flag is set.
What
npm run console -- replay-behavioural-signalsre-scores recorded evidence for a window, or a corpus of fixture shapes, and reports per deployment what fired, what each agreement length would have reached, and how the counts move when the thresholds move. It reads and never writes.