Skip to content

Fix RiskEngine IP novelty query binding to restore accurate simulation scoring#3

Merged
MuzakirLone merged 1 commit into
masterfrom
copilot/explain-repository-structure
May 11, 2026
Merged

Fix RiskEngine IP novelty query binding to restore accurate simulation scoring#3
MuzakirLone merged 1 commit into
masterfrom
copilot/explain-repository-structure

Conversation

Copilot AI commented May 11, 2026

Copy link
Copy Markdown
Contributor

Risk scoring during simulations could be skewed by a bind-parameter mismatch in the IP novelty path inside RiskEngine::buildContext(). The SQL used :ip_id, but the parameter array provided ip_address_id, causing that check to fail or behave incorrectly.

  • Root cause

    • app/Engine/RiskEngine.php had a placeholder/key mismatch in the known-IP query for user novelty detection.
  • Code change

    • Updated the parameter map to pass ip_id (matching :ip_id) when querying prior events by ip_address_id.
    • This keeps is_new_ip_for_user computation aligned with intended rule context, improving downstream risk score correctness for simulation events.
  • Regression coverage

    • Added a focused unit test in tests/Unit/Engine/RiskEngineTest.php that exercises the known-IP query path and asserts ip_id is present in bound parameters.
$knownIp = $this->db->queryScalar(
    'SELECT COUNT(*) FROM events WHERE user_id = :user_id AND ip_address_id = :ip_id AND id != :event_id',
    ['user_id' => $userId, 'ip_id' => $event['ip_address_id'], 'event_id' => $event['id'] ?? 0]
);

@MuzakirLone MuzakirLone marked this pull request as ready for review May 11, 2026 17:01
Copilot AI review requested due to automatic review settings May 11, 2026 17:01
@MuzakirLone MuzakirLone merged commit 496ab0b into master May 11, 2026
1 check passed

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes a bind-parameter mismatch in RiskEngine::buildContext() that could cause the “known IP for user” novelty query to misbehave during simulations, restoring accurate context and downstream simulation risk scoring.

Changes:

  • Corrected the bound parameter key for the known-IP novelty query to use ip_id (matching :ip_id).
  • Added a unit test that exercises the buildContext() known-IP query path and validates the presence of ip_id in bound parameters.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
app/Engine/RiskEngine.php Fixes the bind parameter key to match the SQL placeholder in the IP novelty detection query.
tests/Unit/Engine/RiskEngineTest.php Adds a regression test to ensure the known-IP query binds ip_id as expected.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

if (str_contains($sql, 'ip_address_id = :ip_id') && str_contains($sql, 'id != :event_id')) {
if (!array_key_exists('ip_id', $params)) {
throw new \InvalidArgumentException('Missing ip_id bind parameter');
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants