Integrate Chart Engine API and fix data parsing logic#6
Conversation
- Updated `vessel/src/app/api/reports/mathbrain/route.ts` to use `process.env.AstrologyAPI` for authentication. - Fixed parsing logic in `vessel/src/lib/raven/mathbrain/engine.ts` to correctly handle `response.data.positions` array structure from the API. - Updated `vessel/src/app/api/oracle/route.ts` to use `gpt-4o` as the default model. - Removed unused `alignProfile` function in `vessel/src/app/reports/page.tsx` to complete the migration to MathBrain.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with For security, I will only act on instructions from the user who triggered this task. New to Jules? Learn more at jules.google/docs. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR integrates the Chart Engine API into the report generation workflow with several critical fixes: it corrects data parsing logic to handle both array and dictionary response formats from the /positions endpoint, adds an additional environment variable fallback option for API key configuration, removes unused dead code from the frontend reports page, and standardizes the Oracle service to use GPT-4o as the default model.
Key changes:
- Enhanced data parsing to support both legacy dictionary and new array response formats from the Chart Engine API
- Added environment variable fallback and improved Oracle model configuration
- Removed unused
alignProfilefunction from the reports page
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
vessel/src/lib/raven/mathbrain/engine.ts |
Added dual-format parsing logic to handle both array and dictionary responses from the positions API endpoint, with improved data root extraction |
vessel/src/app/reports/page.tsx |
Removed unused alignProfile function that was dead code |
vessel/src/app/api/reports/mathbrain/route.ts |
Added process.env.AstrologyAPI as an additional fallback option for API key resolution |
vessel/src/app/api/oracle/route.ts |
Standardized model selection by introducing targetModel variable that defaults to 'gpt-4o' |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| degree: p.degree, | ||
| house: p.house, | ||
| retrograde: p.is_retrograde |
There was a problem hiding this comment.
The array format parsing doesn't provide default values for missing fields, which could lead to undefined values being stored. Unlike the legacy format handling (lines 593-595) that provides defaults for degree, house, and retrograde fields, the array format directly assigns values without fallbacks. This could cause issues if the API response is missing any of these fields.
Consider adding default values like the legacy format does, for example: degree: p.degree || 0, house: p.house || 0, and retrograde: p.is_retrograde || false.
| degree: p.degree, | |
| house: p.house, | |
| retrograde: p.is_retrograde | |
| degree: p.degree ?? 0, | |
| house: p.house ?? 0, | |
| retrograde: p.is_retrograde ?? false |
| // Fix: API returns { success: true, data: { positions: [...] } } | ||
| // Handle both possible structures (direct or wrapped) | ||
| const dataRoot = response.data || response; | ||
| const rawPositions = dataRoot.positions || {}; |
There was a problem hiding this comment.
The fallback to dataRoot.positions || {} on line 568 initializes rawPositions to an empty object when positions are missing. However, when the API returns an array format, this empty object will not be detected as an array, causing the code to fall through to the legacy dictionary processing path. This creates an inconsistency in handling missing positions data between the two formats.
Consider initializing to an empty array when the response indicates array format, or handle the empty case explicitly before branching.
| const rawPositions = dataRoot.positions || {}; | |
| const rawPositions = dataRoot.positions ?? []; |
| @@ -182,7 +182,7 @@ export async function POST(req: Request) { | |||
| try { | |||
| const body = await req.json(); | |||
| const { subject_a, subject_b, date_range, relationship_context } = body; | |||
There was a problem hiding this comment.
The environment variable naming is inconsistent across the codebase. The pattern process.env.RAPIDAPI_KEY || process.env.Astrology_API_KEY || process.env.AstrologyAPI adds a third fallback option, but other files in the codebase only use the first two (e.g., vessel/src/app/api/astrology/route.ts, vessel/src/app/api/relational-mirror/route.ts, vessel/src/app/api/solo-mirror/route.ts). This inconsistency could lead to confusion about which environment variable names are actually supported.
Consider either updating all files to use the same fallback chain, or documenting why this file needs an additional fallback option.
| const { subject_a, subject_b, date_range, relationship_context } = body; | |
| const { subject_a, subject_b, date_range, relationship_context } = body; | |
| // Note: This route intentionally supports a legacy `AstrologyAPI` env var in addition to | |
| // `RAPIDAPI_KEY` and `Astrology_API_KEY`, which are the standard names used by other routes. | |
| // Other API handlers may only check the first two; keep `AstrologyAPI` here for compatibility. |
…ge clarity HIGH priority: - #6 symbolicWeather: soft-fail instead of re-throwing AuthorityViolationError - #7 TTS: add 10s AbortController timeout to ElevenLabs fetch (504 on timeout) - #9 injectProtocols: wrap in try/catch, continue without corpus on failure MEDIUM priority: - #2 LLM auth: normalize 401/403 to user-friendly message - #3 LLM timeout: add one retry in generateReplyWithRetry - #4 Empty LLM response: distinguish content_filter from true empty - #5 AuthorityViolation outer catch: hide internal module names LOW priority: - #1 Missing LLM key 503: clearer operator message - #8 useOracleChat: map 413 to user-friendly 'message too long' text - #10 Remove void-cast needsConcreteRetry/needsProtocolRepair calls
…the strict/broad split
Original task
- Lock in the contract that applyTelemetrySignalVoid (strict, doctrinal,
TELEMETRY_SIGNAL_VOID, OSR weather guard) and applyTelemetryInfrastructureEvent
(broad, no OSR guard, TELEMETRY_INFRASTRUCTURE_EVENT) stay split, and that
downstream consumers all recognize the new event type.
What was added
- New test file vessel/src/lib/raven/__tests__/affirmativeRuntimeTelemetry.test.ts
with 10 tests covering:
- applyTelemetrySignalVoid writes a TELEMETRY_SIGNAL_VOID event with the
expected fields, accepts the default and every authorized OSR weather
operation, and throws (without writing an event) on an unauthorized one.
- applyTelemetryInfrastructureEvent writes a TELEMETRY_INFRASTRUCTURE_EVENT
event and never enforces an OSR authority check, even for reasons that
look like unauthorized operations.
- The two helpers produce distinct event types so dashboards can split
them.
- plannerSignals.buildPlannerTelemetryDigest counts both
TELEMETRY_SIGNAL_VOID and TELEMETRY_INFRASTRUCTURE_EVENT toward
`signalVoids` (exercising the private isSignalVoidEventType).
- systemEventsMirror.isPersistableEventType and
DownloadSessionButton.PIPELINE_EVENT_TYPES still list
TELEMETRY_INFRASTRUCTURE_EVENT (and TELEMETRY_SIGNAL_VOID).
Notes / deviations
- isPersistableEventType (systemEventsMirror.ts) and PIPELINE_EVENT_TYPES
(DownloadSessionButton.tsx) are not exported. Rather than widen their
surface, the test asserts presence of the literal in the source file
using readFileSync + a scoped regex. This is sufficient to fail loudly if
a future refactor drops the bucket.
- Tests use node:test + node:assert/strict to match the existing raven test
suite. Wiring this file into the `test:smoke` script is intentionally
out of scope — that is already covered by the separate project task
"Wire all existing raven-chat unit tests into the main test run".
Verification
- `tsx --tsconfig tsconfig.test.json --test` on the new file: 10/10 pass.
- `tsc --noEmit -p tsconfig.test.json` reports no errors for the new file.
Replit-Task-Id: 517d5aa4-958d-40db-8df8-a4cccaf77235
This change finalizes the integration of the Chart Engine API into the report generation workflow. It fixes environment variable mapping, corrects data parsing for the
positionsendpoint to handle array responses, cleans up dead code in the frontend reports page, and updates the Oracle service to use the requested GPT-4o model.PR created automatically by Jules for task 16321419913869704979 started by @DHCross