feat: Plan 6 PR A — network score computation core - #12
Conversation
Add migrations for deferrable score FK, [0,1] weight bounds, and history entity_kind; load/group active attestations; stream RunVeriRank over gRPC; apply scores and sync events under pg_advisory_xact_lock. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds an end-to-end network score recomputation pipeline: active attestation graph loading, observation grouping, TrustEngine gRPC execution with retries, transactional score/history persistence, sync events, trust-weight validation, and database migrations. ChangesNetwork score pipeline
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ScoreComputationService
participant AttestationGraphLoader
participant TrustEngine
participant ScoreWriter
participant PostgreSQL
ScoreComputationService->>AttestationGraphLoader: load graph at evaluationTime
AttestationGraphLoader->>PostgreSQL: query roots, principals, attestations, and score count
ScoreComputationService->>TrustEngine: stream graph to RunVeriRank
TrustEngine-->>ScoreComputationService: return score rows and computed timestamp
ScoreComputationService->>ScoreWriter: apply score table
ScoreWriter->>PostgreSQL: upsert history, emit sync events, and delete stale scores
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
PR Summary by QodoMaterialize network scores through RunVeriRank
AI Description
Diagram
High-Level Assessment
Files changed (19)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
18 rules 1. Stale recompute overwrites newer
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@control-plane/migrations/009_network_scores_fk_deferrable/migration.sql`:
- Around line 4-10: Update the network_scores.principal_id foreign-key
recreation to add it with NOT VALID, then validate it in a separate ALTER TABLE
... VALIDATE CONSTRAINT statement. Keep the existing constraint name, referenced
table, cascade behavior, and deferrable settings unchanged.
In `@control-plane/migrations/010_weight_bounds/migration.sql`:
- Around line 20-28: Update the migration’s constraint replacement for
issuers.trust_weight and bootstrap_issuers.current_weight to add temporary NOT
VALID check constraints, validate each constraint in a controlled step, then
drop the old constraints and rename the validated temporary constraints to the
existing names. Apply the same sequence to both tables while preserving the 0–1
bounds.
In
`@control-plane/migrations/011_network_score_history_entity_kind/migration.sql`:
- Around line 26-31: The migration should avoid blocking full-table scans by
replacing ALTER COLUMN entity_kind SET NOT NULL with a NOT VALID CHECK for
non-null values, validating that constraint, then setting entity_kind NOT NULL;
also add network_score_history_entity_kind_check as NOT VALID followed by
VALIDATE CONSTRAINT.
In `@control-plane/src/domains/graph/attestationGraphLoader.ts`:
- Around line 67-118: Update loadAttestationGraph to acquire one PoolClient and
execute the roots query, attestations query, and principalRows query within a
single transaction using REPEATABLE READ isolation; commit on success and
rollback on failure, releasing the client in all cases. Replace the independent
pool.query calls while preserving the existing evaluationTime and halfLifeCutoff
parameters and graph-building behavior.
In `@control-plane/src/domains/graph/observationGrouping.test.ts`:
- Around line 73-79: Add a test alongside “does not merge different observation
ids” that creates attestations sharing the same observation_id but using
different issuer_id and/or subject_id, then asserts groupAttestationsForScoring
returns both groups. Preserve the existing representative-selection behavior
while covering the cross issuer/subject case.
In `@control-plane/src/domains/graph/scoreWriter.ts`:
- Around line 15-23: Update loadExistingScores to avoid unconditionally loading
the full network_scores table during each recompute; restrict the query to only
scores for principals/entities included in the current recomputation, passing
that scope into the function and using parameterized filtering while preserving
the existing Map<string, ExistingScore> result.
- Around line 114-126: Update the delete branch in the score-writing loop to
insert a final network_score_history row using the pre-delete values from
existing.get(principalId), including entity_kind, score, blacklisted, and
score_reason, before deleting from network_scores. Preserve the existing
score.delete event and deletion flow, and remove the comment indicating deletes
have no history row.
In `@control-plane/src/domains/principal/trustWeight.test.ts`:
- Around line 13-22: Extend the test case around assertTrustWeightInRange to
assert that NaN, Infinity, and -Infinity each throw an AppError with code
CODES.BAD_REQUEST, covering the explicit finite-number validation branch.
In `@control-plane/src/grpc/runVeriRankClient.ts`:
- Around line 118-136: The runVeriRankWithRetry loop currently retries every
error; update its catch logic to retry only errors with transient gRPC statuses
UNAVAILABLE, DEADLINE_EXCEEDED, or RESOURCE_EXHAUSTED. Immediately propagate
permanent errors such as INVALID_ARGUMENT or PERMISSION_DENIED, while preserving
the existing backoff and final-error behavior for retryable failures.
- Around line 74-76: Update the TrustEngine client construction in
runVeriRankClient to select transport credentials based on the configured
config.trustEngine.addr, using secure TLS credentials for non-local or
externally reachable endpoints while retaining insecure credentials only for
explicitly local development addresses. Ensure TRUST_ENGINE_ADDR cannot cause
principal or attestation data to be sent over plaintext on an untrusted network.
- Around line 78-116: Update the RunVeriRank invocation in the surrounding
client method to pass a grpc.Metadata instance as the call metadata argument
instead of the plain object currently containing deadline. Preserve the
60-second deadline by supplying it through the appropriate call options
argument, while leaving the stream writing and response handling unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d2b0b57a-e0ff-475c-8d7a-c3362659ef78
⛔ Files ignored due to path filters (1)
control-plane/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (18)
control-plane/migrations/009_network_scores_fk_deferrable/migration.sqlcontrol-plane/migrations/010_weight_bounds/migration.sqlcontrol-plane/migrations/011_network_score_history_entity_kind/migration.sqlcontrol-plane/package.jsoncontrol-plane/src/config.tscontrol-plane/src/domains/graph/attestationGraphLoader.tscontrol-plane/src/domains/graph/observationGrouping.test.tscontrol-plane/src/domains/graph/observationGrouping.tscontrol-plane/src/domains/graph/scoreComputationService.tscontrol-plane/src/domains/graph/scoreDiff.test.tscontrol-plane/src/domains/graph/scoreDiff.tscontrol-plane/src/domains/graph/scoreWriter.tscontrol-plane/src/domains/principal/principalService.tscontrol-plane/src/domains/principal/trustWeight.test.tscontrol-plane/src/domains/principal/trustWeight.tscontrol-plane/src/domains/sync/syncRepository.tscontrol-plane/src/grpc/runVeriRankClient.tsdocs/superpowers/plans/HANDOVER.md
Pass grpc.Metadata instead of a plain object, honor write backpressure, and load roots/attestations/principals/score-count in one REPEATABLE READ transaction so torn reads cannot clear the score table. Co-authored-by: Cursor <cursoragent@cursor.com>
Fixes for review findings (this push)P0 / your callouts
Also addressed
Deferred to Plan 6 PR B (as planned)
Integration: |
messagesgoel-blip
left a comment
messagesgoel-blip
left a comment
There was a problem hiding this comment.
Re-review of commit 2a6ceaf:
- Validated the original two blockers as fixed:
RunVeriRanknow receivesgrpc.Metadataplus call options and honors stream backpressure; graph roots, attestations, principals, and score count now share oneREPEATABLE READsnapshot. - The retry and loopback/TLS changes are present, the additional grouping and non-finite weight tests are present, and all four CI jobs are green on this head. The reported local integration result is 9/9.
- Single-flight remains intentionally deferred to PR B, matching the locked plan.
- The score-delete history and scoped score-load suggestions should be closed with design rationale: Decisions 10/12 require no history row on delete, and the writer needs the complete existing-score set to identify stale rows.
One remediation is incomplete: migration 011_network_score_history_entity_kind still executes ALTER COLUMN entity_kind SET NOT NULL before establishing and validating a temporary non-null check. The later NOT VALID check only validates the enum values, so it does not remove the blocking full-table scan called out in the review. If production-safe migration behavior is required, add CHECK (entity_kind IS NOT NULL) NOT VALID, validate it, then set the column NOT NULL.
Outcome: fixes validated; the migration-011 item above is the only remaining actionable issue from this pass. Submitted as a comment review because the authenticated account is the PR author and GitHub does not permit self-approval.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
control-plane/migrations/011_network_score_history_entity_kind/migration.sql (1)
26-27: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPast review comment only half-addressed:
SET NOT NULLstill causes a blocking table scan.The prior request was to add a
NOT VALIDCHECK for non-null, validate it, then runSET NOT NULL(so Postgres can skip the redundant scan). Theentity_kind_checkCHECK constraint below was split intoNOT VALID+VALIDATE CONSTRAINT, but thisALTER COLUMN entity_kind SET NOT NULLwas left unchanged and still forces a full-table scan under a blocking lock.🔧 Proposed fix
+ALTER TABLE network_score_history + ADD CONSTRAINT network_score_history_entity_kind_not_null + CHECK (entity_kind IS NOT NULL) NOT VALID; + +ALTER TABLE network_score_history + VALIDATE CONSTRAINT network_score_history_entity_kind_not_null; + ALTER TABLE network_score_history ALTER COLUMN entity_kind SET NOT NULL; + +ALTER TABLE network_score_history + DROP CONSTRAINT network_score_history_entity_kind_not_null;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@control-plane/migrations/011_network_score_history_entity_kind/migration.sql` around lines 26 - 27, Update the migration’s entity_kind constraint sequence: after validating the existing entity_kind_check constraint, change the ALTER COLUMN entity_kind SET NOT NULL operation to the PostgreSQL form that reuses the validated CHECK constraint and avoids another blocking table scan. Preserve the prior NOT VALID and VALIDATE CONSTRAINT steps.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@control-plane/migrations/009_network_scores_fk_deferrable/migration.sql`:
- Around line 7-14: Remove the VALIDATE CONSTRAINT statement from
control-plane/migrations/009_network_scores_fk_deferrable/migration.sql, leaving
the foreign key creation as NOT VALID, and add its validation to a later
migration. Likewise remove validation of network_score_history_entity_kind_check
from
control-plane/migrations/011_network_score_history_entity_kind/migration.sql and
place it in a later migration, so both scans run outside their creation
migrations’ lock windows.
In `@control-plane/migrations/010_weight_bounds/migration.sql`:
- Around line 20-24: Rework the migration’s constraint replacement sequence so
the new uniquely named NOT VALID trust-weight constraints are added first and
validated in a separate transaction, confirming the migration runner supports
that transaction boundary. Only after validation, use a short transaction to
drop the existing issuers trust-weight constraints and rename the validated
replacements to the original constraint names; apply the same ordering to the
additional constraint block.
In `@control-plane/src/grpc/runVeriRankClient.ts`:
- Line 129: Update the deadline calculation in runVeriRankClient to avoid a
fixed 60-second timeout: source the timeout from the existing configuration if
available, or scale it using the graph’s principals and attestations counts to
accommodate drain backpressure for larger graphs. Preserve the current deadline
behavior for small graphs and ensure the computed deadline remains a valid
future Date.
- Around line 71-77: Update writeChunk so the wait after call.write(chunk)
returns false races the 'drain' event against stream 'error' and 'close' events,
rejecting or otherwise terminating promptly when either failure event occurs
instead of hanging. Preserve the immediate return when write succeeds.
---
Outside diff comments:
In
`@control-plane/migrations/011_network_score_history_entity_kind/migration.sql`:
- Around line 26-27: Update the migration’s entity_kind constraint sequence:
after validating the existing entity_kind_check constraint, change the ALTER
COLUMN entity_kind SET NOT NULL operation to the PostgreSQL form that reuses the
validated CHECK constraint and avoids another blocking table scan. Preserve the
prior NOT VALID and VALIDATE CONSTRAINT steps.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7637f2b8-247b-49f1-82f0-8d9cf55f957d
📒 Files selected for processing (8)
control-plane/migrations/009_network_scores_fk_deferrable/migration.sqlcontrol-plane/migrations/010_weight_bounds/migration.sqlcontrol-plane/migrations/011_network_score_history_entity_kind/migration.sqlcontrol-plane/src/domains/graph/attestationGraphLoader.tscontrol-plane/src/domains/graph/observationGrouping.test.tscontrol-plane/src/domains/graph/scoreComputationService.tscontrol-plane/src/domains/principal/trustWeight.test.tscontrol-plane/src/grpc/runVeriRankClient.ts
Validate replacement FK/CHECK constraints before dropping live ones, use a validated NOT NULL check before SET NOT NULL, and race drain against stream error/close so writeChunk cannot hang. Co-authored-by: Cursor <cursoragent@cursor.com>
Follow-up fixes (drain + migration ordering)
Unit + integration still green locally. |
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 (1)
control-plane/migrations/011_network_score_history_entity_kind/migration.sql (1)
7-17: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not populate historical rows from only current-state kinds.
This assigns every pre-migration history row the current
network_scores.entity_kindorprincipals.entity_kind. If a principal changed kind, earlier history is rewritten with the new kind and cannot audit that change. Use an authoritative historical source, or explicitly document and test that pre-011 rows are only current-state backfills.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@control-plane/migrations/011_network_score_history_entity_kind/migration.sql` around lines 7 - 17, Revise the migration’s entity_kind backfill statements for network_score_history so historical rows are not assigned solely from current network_scores or principals values. Use an authoritative historical source that preserves each row’s kind; if unavailable, explicitly document the current-state backfill limitation and add tests covering principals whose kind changed before migration 011.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@control-plane/migrations/009_network_scores_fk_deferrable/migration.sql`:
- Around line 11-12: Separate constraint creation from validation so validation
does not run within the originating migration transaction. In
control-plane/migrations/009_network_scores_fk_deferrable/migration.sql lines
11-12, move network_scores_principal_id_fkey_v2 validation to a later migration;
do the same for the temporary non-null and entity-kind constraints in
control-plane/migrations/011_network_score_history_entity_kind/migration.sql
lines 31-32 and 44-45, and the issuer and bootstrap weight constraints in
control-plane/migrations/010_weight_bounds/migration.sql lines 24 and 31.
In `@control-plane/src/grpc/runVeriRankClient.ts`:
- Around line 76-85: The Promise.race in the backpressure handling flow must
remove losing event listeners after settlement. Update the logic around call and
the drain/error/close listeners to attach handlers once, settle on the first
event, and clean up all remaining listeners whether drain, error, or close wins,
while preserving the existing error propagation and closed-stream failure
behavior.
---
Outside diff comments:
In
`@control-plane/migrations/011_network_score_history_entity_kind/migration.sql`:
- Around line 7-17: Revise the migration’s entity_kind backfill statements for
network_score_history so historical rows are not assigned solely from current
network_scores or principals values. Use an authoritative historical source that
preserves each row’s kind; if unavailable, explicitly document the current-state
backfill limitation and add tests covering principals whose kind changed before
migration 011.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2c8bb667-d007-418c-ad89-2a32f09623f3
📒 Files selected for processing (4)
control-plane/migrations/009_network_scores_fk_deferrable/migration.sqlcontrol-plane/migrations/010_weight_bounds/migration.sqlcontrol-plane/migrations/011_network_score_history_entity_kind/migration.sqlcontrol-plane/src/grpc/runVeriRankClient.ts
messagesgoel-blip
left a comment
messagesgoel-blip
left a comment
There was a problem hiding this comment.
Re-review of commit 98b73d9:
CI run 28 is green, and the intended fixes are directionally correct, but two major issues remain:
-
writeChunknow racesdrain,error, andclose, but the losingonce()listeners are not removed whendrainwins. Every backpressured write leaves anerrorandcloselistener attached until stream termination, so a large graph accumulates listeners and closures and can triggerMaxListenersExceededWarning. Use one promise with explicit handlers and a shared cleanup function, or abort the losing listeners after the first event settles. -
The migration runner wraps every
migration.sqlfile in a singleBEGIN/COMMIT. Adding aNOT VALIDconstraint and validating it later in the same file therefore keeps the stronger creation lock until validation finishes, defeating the production-safe lock reduction. Move validation and the subsequent drop/rename into later migration transactions for 009 and 010; similarly validate the temporary non-null and enum checks from 011 in a later migration beforeSET NOT NULL/cleanup.
The dynamic deadline change is reasonable, all tests are green, and single-flight remains correctly scoped to Plan 6 PR B. Outcome: two blocking remediations remain. Submitted as a comment review because GitHub does not allow the PR author account to self-approve.
Replace Promise.race(once) with cleaned-up drain/error/close handlers, and move VALIDATE/drop/rename/SET NOT NULL into migration 012 so validation scans run in a separate migration transaction. Co-authored-by: Cursor <cursoragent@cursor.com>
Remaining blockers fixed
Plan 6 doc updated for |
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 (1)
control-plane/migrations/011_network_score_history_entity_kind/migration.sql (1)
8-18: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not backfill historical kinds from current state.
network_score_historyis keyed by(principal_id, sync_version)(control-plane/migrations/002_attestations/migration.sql, Lines 50-58), but these updates join only by principal and read the latestnetwork_scores/principalskind. Ifentity_kindchanged over time—as the plan permits—every older history row receives the current kind, making the audit trail incorrect. Backfill from a source versioned bysync_version; otherwise require an explicit legacy-data policy instead of claiming an exact historical backfill.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@control-plane/migrations/011_network_score_history_entity_kind/migration.sql` around lines 8 - 18, The migration’s entity_kind backfill incorrectly applies current network_scores or principals values to every historical row for a principal. Remove these non-versioned updates and either backfill network_score_history using a source keyed/versioned by sync_version, or adopt an explicit legacy-data policy that does not claim exact historical reconstruction.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@control-plane/migrations/011_network_score_history_entity_kind/migration.sql`:
- Around line 8-18: The migration’s entity_kind backfill incorrectly applies
current network_scores or principals values to every historical row for a
principal. Remove these non-versioned updates and either backfill
network_score_history using a source keyed/versioned by sync_version, or adopt
an explicit legacy-data policy that does not claim exact historical
reconstruction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4d2f6ef8-677c-410a-a216-6fa33778d09d
📒 Files selected for processing (7)
control-plane/migrations/009_network_scores_fk_deferrable/migration.sqlcontrol-plane/migrations/010_weight_bounds/migration.sqlcontrol-plane/migrations/011_network_score_history_entity_kind/migration.sqlcontrol-plane/migrations/012_validate_score_constraints/migration.sqlcontrol-plane/src/grpc/runVeriRankClient.test.tscontrol-plane/src/grpc/runVeriRankClient.tsdocs/superpowers/plans/2026-07-28-network-score-computation.md
Co-authored-by: Cursor <cursoragent@cursor.com>
Review threads resolvedChecked HEAD Resolved all open threads:
CodeRabbit entity_kind backfill outside-diff note remains dismissed per locked Plan 6 current-state backfill policy. |
1 participant
Summary
009–011: deferrablenetwork_scoresFK, weight[0,1]checks,network_score_history.entity_kindevaluationTimeobservation_idgrouping; score writer withpg_advisory_xact_lock+score.upsert/score.deleteRunVeriRankclient (@grpc/proto-loader);recomputeNoworchestrationtrust_weightvalidation in[0,1]; syncappendEventuses xact lockOut of this PR (Plan 6 PR B): single-flight scheduler, ingest
markDirty, mandatory CI trust-engine integration test.Test plan
cd control-plane && npm run test:unitverilink_test(port 15432)recomputeNowagainst seeded graph@coderabbitai review
Summary by CodeRabbit