fix: harden telemetry failure and source projection - #3
Merged
Merged
Conversation
📝 WalkthroughWalkthroughChangesArticle link classification
Intelligence pipeline updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CandidateEvidence
participant adjudicateEvidence
participant Adjudicator
CandidateEvidence->>adjudicateEvidence: candidate batches of up to six
adjudicateEvidence->>Adjudicator: truncated batch prompt
Adjudicator-->>adjudicateEvidence: batch decisions
adjudicateEvidence-->>CandidateEvidence: aggregated decisions
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR hardens the intelligence pipeline’s terminal failure handling to avoid masking underlying errors, tightens evidence adjudication constraints, and refines “Works Cited” source projection to exclude non-editorial link types.
Changes:
- Add a bounded fallback message for terminal pipeline failures when an upstream error message is empty.
- Make evidence adjudication more tolerant (nullable defaults) and more constrained (smaller output + batched adjudication calls).
- Exclude same-publication links from the projected source list (keeping only external and likely-primary sources).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/intelligence/src/report/projector.ts | Updates source projection to include only external and likely-primary links. |
| packages/intelligence/src/report/projector.test.ts | Updates expected projected sources to match the new filtering. |
| packages/intelligence/src/pipeline.ts | Adds pipelineErrorMessage to ensure terminal failure events always have a valid bounded message. |
| packages/intelligence/src/pipeline.test.ts | Adds coverage for empty-error provider failures producing a valid terminal failure event. |
| packages/intelligence/src/evidence/adjudication.ts | Reduces adjudication output size and introduces batching for adjudication calls. |
| packages/extraction/src/article-index.ts | Expands link classification to better label social/promotional/non-editorial hosts. |
| packages/extraction/src/article-index.test.ts | Adds tests ensuring social/promotional links are classified correctly. |
| packages/contracts/src/evidence.ts | Makes nullable adjudication fields default to null when omitted. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+115
to
+119
| const maxCandidatesPerCall = 6; | ||
| const decisions: EvidenceAdjudication[] = []; | ||
| for (let offset = 0; offset < input.candidates.length; offset += maxCandidatesPerCall) { | ||
| const batch = input.candidates.slice(offset, offset + maxCandidatesPerCall); | ||
| decisions.push( |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/intelligence/src/pipeline.test.ts (1)
178-199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the new normalization branches.
This test covers only an empty
Errorduring analysis. Add cases for a thrown string, a whitespace-only message, a message longer than 1,000 characters, and a targeted-retry failure.🤖 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 `@packages/intelligence/src/pipeline.test.ts` around lines 178 - 199, Extend the analysis failure tests around analyzeArticle to cover normalization of a thrown string, a whitespace-only Error message, and an Error message exceeding 1,000 characters, asserting each produces the expected terminal failure message. Also add a targeted-retry failure case using the existing retry configuration or symbols, and verify its normalized failure event behavior.
🤖 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 `@packages/extraction/src/article-index.ts`:
- Line 207: Update the non-editorial host check in the link classification logic
around NON_EDITORIAL_HOSTS and PROMOTIONAL_PATTERN so it matches both each
configured apex domain and its subdomains, consistent with the SOCIAL_HOSTS
matching behavior. Ensure hosts such as www.beyondwords.io are treated as
non-editorial before they can enter the external-source projection.
In `@packages/intelligence/src/evidence/adjudication.ts`:
- Line 13: Update the aggregation logic in adjudicator.adjudicate so the public
result is capped at eight decisions, not just each individual batch response.
After combining batch results, apply a deterministic selection rule (preserving
the established decision order) before returning the aggregate, and add a
regression test covering multiple batches that would otherwise exceed the limit.
- Around line 115-126: Update adjudicateEvidence to compute one absolute
deadline from input.budget.totalDeadlineMs, derive the remaining time before
each batch, and pass a budget with totalDeadlineMs clamped to that remaining
duration into input.adjudicator.adjudicate. Preserve the existing six-candidate
batching and decision aggregation while ensuring every batch and retry shares
the pipeline deadline.
---
Nitpick comments:
In `@packages/intelligence/src/pipeline.test.ts`:
- Around line 178-199: Extend the analysis failure tests around analyzeArticle
to cover normalization of a thrown string, a whitespace-only Error message, and
an Error message exceeding 1,000 characters, asserting each produces the
expected terminal failure message. Also add a targeted-retry failure case using
the existing retry configuration or symbols, and verify its normalized failure
event behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 025b6cf4-7888-49f0-920b-367c4b722b08
📒 Files selected for processing (8)
packages/contracts/src/evidence.tspackages/extraction/src/article-index.test.tspackages/extraction/src/article-index.tspackages/intelligence/src/evidence/adjudication.tspackages/intelligence/src/pipeline.test.tspackages/intelligence/src/pipeline.tspackages/intelligence/src/report/projector.test.tspackages/intelligence/src/report/projector.ts
| return "social"; | ||
| } | ||
| if (/\b(?:subscribe|newsletter|advert|sponsor|shop|store)\b/i.test(`${link.label} ${link.url}`)) { | ||
| if (NON_EDITORIAL_HOSTS.has(linkHost) || PROMOTIONAL_PATTERN.test(`${link.label} ${link.url}`)) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Match non-editorial host subdomains.
NON_EDITORIAL_HOSTS.has(linkHost) only matches beyondwords.io. A link such as https://www.beyondwords.io/... classifies as external and can reach Works Cited through the external-source projection. Match the apex domain and its subdomains, as SOCIAL_HOSTS already does.
Proposed fix
- if (NON_EDITORIAL_HOSTS.has(linkHost) || PROMOTIONAL_PATTERN.test(`${link.label} ${link.url}`)) {
+ if (
+ [...NON_EDITORIAL_HOSTS].some(
+ (domain) => linkHost === domain || linkHost.endsWith(`.${domain}`),
+ ) ||
+ PROMOTIONAL_PATTERN.test(`${link.label} ${link.url}`)
+ ) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (NON_EDITORIAL_HOSTS.has(linkHost) || PROMOTIONAL_PATTERN.test(`${link.label} ${link.url}`)) { | |
| if ( | |
| [...NON_EDITORIAL_HOSTS].some( | |
| (domain) => linkHost === domain || linkHost.endsWith(`.${domain}`), | |
| ) || | |
| PROMOTIONAL_PATTERN.test(`${link.label} ${link.url}`) | |
| ) { |
🤖 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 `@packages/extraction/src/article-index.ts` at line 207, Update the
non-editorial host check in the link classification logic around
NON_EDITORIAL_HOSTS and PROMOTIONAL_PATTERN so it matches both each configured
apex domain and its subdomains, consistent with the SOCIAL_HOSTS matching
behavior. Ensure hosts such as www.beyondwords.io are treated as non-editorial
before they can enter the external-source projection.
|
|
||
| const AdjudicationOutputSchema = z.object({ | ||
| decisions: z.array(EvidenceAdjudicationSchema).max(96), | ||
| decisions: z.array(EvidenceAdjudicationSchema).max(8), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Enforce the eight-decision limit on the public result.
Line 13 limits each adjudicator.adjudicate response. Lines 115-126 append every batch response and return the aggregate without a cap. Multiple batches can therefore return more than eight decisions, which conflicts with the bounded-output objective.
Apply one aggregate limit and a deterministic selection rule. Add a regression test with multiple batches.
Suggested aggregate limit
+const MAX_DECISIONS = 8;
+
const AdjudicationOutputSchema = z.object({
- decisions: z.array(EvidenceAdjudicationSchema).max(8),
+ decisions: z.array(EvidenceAdjudicationSchema).max(MAX_DECISIONS),
});
...
- return decisions;
+ return decisions.slice(0, MAX_DECISIONS);Also applies to: 115-126
🤖 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 `@packages/intelligence/src/evidence/adjudication.ts` at line 13, Update the
aggregation logic in adjudicator.adjudicate so the public result is capped at
eight decisions, not just each individual batch response. After combining batch
results, apply a deterministic selection rule (preserving the established
decision order) before returning the aggregate, and add a regression test
covering multiple batches that would otherwise exceed the limit.
Comment on lines
+115
to
+126
| const maxCandidatesPerCall = 6; | ||
| const decisions: EvidenceAdjudication[] = []; | ||
| for (let offset = 0; offset < input.candidates.length; offset += maxCandidatesPerCall) { | ||
| const batch = input.candidates.slice(offset, offset + maxCandidatesPerCall); | ||
| decisions.push( | ||
| ...(await input.adjudicator.adjudicate({ | ||
| ...input, | ||
| candidates: batch, | ||
| })), | ||
| ); | ||
| } | ||
| return decisions; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 8 \
-e '\btotalDeadlineMs\b' \
-e '\bAbortController\b' \
-e '\bdeadline\b' \
-e '\badjudicateEvidence\b' \
packages/intelligence packages/contracts --glob '*.ts'Repository: drperky20/perspectica
Length of output: 18822
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "== files =="
git ls-files 'packages/intelligence/src/**/*.ts' | sed -n '1,120p'
echo
echo "== adjudication.ts outline =="
ast-grep outline packages/intelligence/src/evidence/adjudication.ts --view expanded || true
echo
echo "== adjudication.ts 1-150 =="
cat -n packages/intelligence/src/evidence/adjudication.ts | sed -n '1,150p'
echo
echo "== budgets.ts =="
cat -n packages/intelligence/src/budgets.ts | sed -n '1,120p'
echo
echo "== exact pipeline/adjudicator usages =="
rg -n -C 4 'adjudicateEvidence|budget: resolveAnalysisBudget|resolveAnalysisBudget\(' packages/intelligence/src --glob '*.ts'Repository: drperky20/perspectica
Length of output: 15415
Carry a remaining deadline for each adjudication batch.
adjudicateEvidence creates 6-candidate batches and passes the same input.budget into each batch. createModelEvidenceAdjudicator uses input.budget.totalDeadlineMs for every generateText timeout, so multiple batches plus one retry each can exceed the pipeline deadline. Track a single absolute deadline and clamp each batch timeout to the remaining time before passing it to adjudicator.adjudicate.
🤖 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 `@packages/intelligence/src/evidence/adjudication.ts` around lines 115 - 126,
Update adjudicateEvidence to compute one absolute deadline from
input.budget.totalDeadlineMs, derive the remaining time before each batch, and
pass a budget with totalDeadlineMs clamped to that remaining duration into
input.adjudicator.adjudicate. Preserve the existing six-candidate batching and
decision aggregation while ensuring every batch and retry shares the pipeline
deadline.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Root cause
The supplied telemetry reached the pipeline catch path with an empty
Error.message. The catch emitted that empty value, andPipelineEventSchema.parsethen failed on the required nonemptydata.message, masking the original failure. The pipeline now guarantees a bounded fallback message.Validation
pnpm format:checkpnpm typecheckpnpm test— 26 files, 109 testspnpm --filter @perspectica/extension buildpnpm bench:v2git diff --checkNo new live authenticated provider analysis was run in this environment; the failure handling fix is directly based on the supplied Fox News telemetry.
Summary by CodeRabbit