Classify candidate rejection feedback with Bragi#3563
Classify candidate rejection feedback with Bragi#3563davidercruz wants to merge 18 commits intomainfrom
Conversation
|
🍹 The Update (preview) for dailydotdev/api/prod (at 679f439) was successful. Resource Changes Name Type Operation
~ vpc-native-update-source-public-threshold-cron kubernetes:batch/v1:CronJob update
~ vpc-native-update-current-streak-cron kubernetes:batch/v1:CronJob update
~ vpc-native-user-profile-updated-sync-cron kubernetes:batch/v1:CronJob update
~ vpc-native-personalized-digest-deployment kubernetes:apps/v1:Deployment update
~ vpc-native-calculate-top-readers-cron kubernetes:batch/v1:CronJob update
~ vpc-native-bg-deployment kubernetes:apps/v1:Deployment update
~ vpc-native-update-source-tag-view-cron kubernetes:batch/v1:CronJob update
~ vpc-native-clean-zombie-users-cron kubernetes:batch/v1:CronJob update
~ vpc-native-private-deployment kubernetes:apps/v1:Deployment update
~ vpc-native-update-highlighted-views-cron kubernetes:batch/v1:CronJob update
~ vpc-native-clean-zombie-images-cron kubernetes:batch/v1:CronJob update
~ vpc-native-post-analytics-clickhouse-cron kubernetes:batch/v1:CronJob update
~ vpc-native-deployment kubernetes:apps/v1:Deployment update
~ vpc-native-generic-referral-reminder-cron kubernetes:batch/v1:CronJob update
~ vpc-native-sync-subscription-with-cio-cron kubernetes:batch/v1:CronJob update
~ vpc-native-post-analytics-history-day-clickhouse-cron kubernetes:batch/v1:CronJob update
~ vpc-native-user-profile-analytics-clickhouse-cron kubernetes:batch/v1:CronJob update
~ vpc-native-clean-zombie-user-companies-cron kubernetes:batch/v1:CronJob update
~ vpc-native-clean-zombie-opportunities-cron kubernetes:batch/v1:CronJob update
~ vpc-native-expire-super-agent-trial-cron kubernetes:batch/v1:CronJob update
~ vpc-native-temporal-deployment kubernetes:apps/v1:Deployment update
- vpc-native-api-clickhouse-migration-05196779 kubernetes:batch/v1:Job delete
~ vpc-native-update-tag-recommendations-cron kubernetes:batch/v1:CronJob update
~ vpc-native-clean-gifted-plus-cron kubernetes:batch/v1:CronJob update
~ vpc-native-daily-digest-cron kubernetes:batch/v1:CronJob update
~ vpc-native-update-views-cron kubernetes:batch/v1:CronJob update
~ vpc-native-generate-search-invites-cron kubernetes:batch/v1:CronJob update
~ vpc-native-update-tags-str-cron kubernetes:batch/v1:CronJob update
~ vpc-native-clean-stale-user-transactions-cron kubernetes:batch/v1:CronJob update
+ vpc-native-api-db-migration-3de9eb51 kubernetes:batch/v1:Job create
~ vpc-native-user-profile-analytics-history-clickhouse-cron kubernetes:batch/v1:CronJob update
~ vpc-native-user-posts-analytics-refresh-cron kubernetes:batch/v1:CronJob update
~ vpc-native-check-analytics-report-cron kubernetes:batch/v1:CronJob update
~ vpc-native-hourly-notification-cron kubernetes:batch/v1:CronJob update
~ vpc-native-validate-active-users-cron kubernetes:batch/v1:CronJob update
~ vpc-native-personalized-digest-cron kubernetes:batch/v1:CronJob update
~ vpc-native-update-trending-cron kubernetes:batch/v1:CronJob update
~ vpc-native-ws-deployment kubernetes:apps/v1:Deployment update
- vpc-native-api-db-migration-05196779 kubernetes:batch/v1:Job delete
+ vpc-native-api-clickhouse-migration-3de9eb51 kubernetes:batch/v1:Job create
... and 1 other changes |
ce4fe86 to
b0e932a
Compare
| public async up(queryRunner: QueryRunner): Promise<void> { | ||
| await queryRunner.query( | ||
| `ALTER TABLE "opportunity_match" | ||
| ADD "rejectionClassification" jsonb DEFAULT null`, |
There was a problem hiding this comment.
Should also modify here then
| value?: unknown; | ||
| }; | ||
|
|
||
| const mapPreferenceFields = ( |
| ? `${opportunity.title}\n${opportunity.tldr}` | ||
| : undefined; | ||
|
|
||
| const result = await bragiClient.garmr.execute(() => |
There was a problem hiding this comment.
This flow is a bit weird, you should probably do this where we request the feedback? This only happens once we get feedback so it won't even trigger for rejection I don't think.
| if (err instanceof ConnectError) { | ||
| logger.error( | ||
| { err, opportunityId, userId, answer: item.answer }, | ||
| 'ConnectError when parsing feedback', | ||
| ); | ||
| return item; | ||
| } | ||
| throw err; |
There was a problem hiding this comment.
Since you are using Promise.all, throw on any of them will cancel result for all of them, so I think its better to not throw and ignore errors from bragi, there are retries already.
| public async up(queryRunner: QueryRunner): Promise<void> { | ||
| await queryRunner.query( | ||
| `ALTER TABLE "opportunity_match" | ||
| ADD "rejectionClassification" jsonb`, |
There was a problem hiding this comment.
add column if not exists, also for drop
| }); | ||
|
|
||
| if (!match) { | ||
| logger.warn({ opportunityId, userId }, 'No match found for feedback'); |
There was a problem hiding this comment.
you can make all of this warns debug (here and below), no point in spamming logs, agent should know this already 😫
| // Skip rejection classification if already set (idempotent) | ||
| if (match.rejectionClassification) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
With #3540 there can be multiple feedbacks, so potentially we need to overwrite it and then save current result in history column
bdb75a7 to
384f737
Compare
Co-authored-by: Ole-Martin Bratteng <1681525+omBratteng@users.noreply.github.com>
- Don't throw errors in Promise.all to avoid cancelling sibling results - Change warn/error logs to debug to reduce log noise - Allow overwriting rejectionClassification for rematch support - Remove ConnectError-specific handling in favor of catching all errors Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
e07d9f9 to
126d987
Compare
| export type RejectionReasonDetail = z.infer<typeof rejectionReasonDetailSchema>; | ||
| export type RejectionFeedbackClassification = z.infer< | ||
| typeof rejectionFeedbackClassificationSchema | ||
| >; |
There was a problem hiding this comment.
@claude I told you already to not rexport z.infer as types, you should just use z.infer on all places and import schema, add this to AGENTS.md:
**IMPORTANT - Zod Type Inference:**
- **ALWAYS use `z.infer` to derive TypeScript types from Zod schemas**
- **NEVER manually define types that duplicate Zod schema structure**
There was a problem hiding this comment.
Claude finished @capJavert's task in 3m 2s —— View job
Adding Zod Type Inference Guidelines
I've added the guideline to AGENTS.md and refactored the code to follow the pattern.
What Changed:
-
Added guideline to AGENTS.md in the "Zod patterns" section:
- Always use
z.infer<typeof schema>at the point of use - Never re-export types from Zod schemas
- Export only the schemas themselves
- Always use
-
Removed type re-exports from
src/common/schema/opportunityMatch.ts:- Removed
OpportunityFeedback,FeedbackClassification,RejectionReasonDetail,RejectionFeedbackClassification, andAnonymousUserContexttype exports - Kept only the schema exports
- Removed
-
Updated all imports to use
z.infer<typeof schema>directly:src/entity/OpportunityMatch.ts- Updated entity type definitionssrc/workers/opportunity/parseOpportunityFeedback.ts- Updated worker function types
Tasks Completed:
- Read AGENTS.md to find the best location for the new guideline
- Read the current code in
src/common/schema/opportunityMatch.ts - Find all usages of re-exported types
- Add guideline to AGENTS.md
- Remove type re-exports from
src/common/schema/opportunityMatch.ts - Update files to use
z.infer<typeof schema>directly - Commit and push changes
Commit: 9dff651
| const preference = | ||
| reason.preference?.case === 'locationTypePreference' | ||
| ? { locationTypePreference: reason.preference.value } | ||
| : reason.preference?.case === 'seniorityPreference' | ||
| ? { seniorityPreference: reason.preference.value } | ||
| : reason.preference?.case === 'employmentTypePreference' | ||
| ? { employmentTypePreference: reason.preference.value } | ||
| : reason.preference?.case === 'freeTextPreference' | ||
| ? { freeTextPreference: reason.preference.value } | ||
| : {}; |
There was a problem hiding this comment.
Claude finished @capJavert's task in 28s —— View job
Analyzing if-else vs switch case
Tasks:
- Read the file to see the current implementation
- Analyze the structure and provide feedback
- Update comment with findings
You're right that the nested ternary chain on lines 133-142 could be more readable as a switch statement. The current code uses chained ternary operators to map reason.preference?.case to different preference object structures.
Current code (nested ternary):
const preference =
reason.preference?.case === 'locationTypePreference'
? { locationTypePreference: reason.preference.value }
: reason.preference?.case === 'seniorityPreference'
? { seniorityPreference: reason.preference.value }
: reason.preference?.case === 'employmentTypePreference'
? { employmentTypePreference: reason.preference.value }
: reason.preference?.case === 'freeTextPreference'
? { freeTextPreference: reason.preference.value }
: {};Would be clearer as a switch:
let preference = {};
if (reason.preference?.case) {
switch (reason.preference.case) {
case 'locationTypePreference':
preference = { locationTypePreference: reason.preference.value };
break;
case 'seniorityPreference':
preference = { seniorityPreference: reason.preference.value };
break;
case 'employmentTypePreference':
preference = { employmentTypePreference: reason.preference.value };
break;
case 'freeTextPreference':
preference = { freeTextPreference: reason.preference.value };
break;
}
}The switch statement is more readable and easier to extend. However, according to CLAUDE.md, this codebase prefers const arrow functions and concise implementations. The ternary is more concise but less maintainable.
Would you like me to refactor this to use a switch statement?
- Remove type re-exports from src/common/schema/opportunityMatch.ts - Update all imports to use z.infer<typeof schema> at point of use - Add guideline to AGENTS.md documenting this pattern Co-authored-by: Ante Barić <capJavert@users.noreply.github.com>
| } catch (err) { | ||
| logger.debug( | ||
| { err, opportunityId, userId }, | ||
| 'Error when classifying rejection feedback', | ||
| ); |
There was a problem hiding this comment.
@claude you swallowed the error now, lets just remove try catch and let worker retry and notify us automatically as usual
There was a problem hiding this comment.
Claude finished @capJavert's task in 1m 26s —— View job
Removing try-catch to allow worker retry
Tasks:
- Read the current implementation to identify the try-catch block
- Remove the try-catch block to let errors propagate
- Run lint to verify code quality (will run in CI)
- Commit and push changes
Summary
Removed the try-catch block (lines 100-167) that was wrapping the rejection classification logic. Now when errors occur in the rejection classification process, they will propagate naturally to the worker, triggering automatic retries and notifications instead of being silently logged.
Changes:
- Removed try-catch wrapper around rejection classification logic
- Errors now propagate to worker for automatic retry behavior
- No more silent failure logging
Commit: 2f5ffb9
| Branch
There was a problem hiding this comment.
@claude Also note this pattern down in agents.md please we keep telling you
There was a problem hiding this comment.
Claude finished @rebelchris's task in 1m 9s —— View job
Adding Pattern Documentation to AGENTS.md
Tasks:
- Read the specific code pattern in parseOpportunityFeedback.ts
- Read AGENTS.md to find the appropriate section
- Add guideline about preferring switch statements over nested ternary operators
- Commit and push changes
- Update comment with results
Summary
I've added a guideline to AGENTS.md in the "Keep implementations concise" section about preferring switch statements over nested ternary operators when mapping 3+ conditional branches.
What Changed:
- Added guideline with a clear BAD/GOOD example showing:
- BAD: Nested ternary chain that's hard to read and extend
- GOOD: Switch statement that's clear and maintainable
- The guideline specifically addresses the pattern in
parseOpportunityFeedback.ts:133-142where nested ternary operators mapreason.preference?.caseto different preference objects
Commit: 6bb792d
--- | Branch
| logger.info( | ||
| { opportunityId, userId }, | ||
| 'Successfully parsed opportunity feedback', | ||
| ); |
There was a problem hiding this comment.
@claude remove it, no need to log, we can see clarification in the DB, also add to AGENTS.md that useless logger.info calls are not needed in success situations like this
There was a problem hiding this comment.
Claude finished @capJavert's task in 1m 44s —— View job
Removing unnecessary logger.info and adding guideline
Tasks:
- Read the file to see the logger.info call at line 94
- Read AGENTS.md to find appropriate location for guideline
- Remove the logger.info calls
- Add guideline to AGENTS.md about avoiding unnecessary logger.info calls
- Run lint to verify code quality
- Commit and push changes
What Changed:
-
Removed two logger.info calls from success paths:
- After parsing opportunity feedback (line 95-98)
- After classifying rejection feedback (line 158-161)
-
Added guideline to AGENTS.md in the "Keep implementations concise" section:
- "Never use logger.info for successful operations - successful database updates, API calls, or data processing don't need logging. Results are visible in the database and errors will propagate naturally with automatic retry notifications."
Success operations don't need logging since the results are visible in the database and any errors will automatically propagate with retry notifications.
Commit: ad0773f
Co-authored-by: Ante Barić <capJavert@users.noreply.github.com>
Success operations don't need logging as results are visible in DB and errors propagate naturally. Added guideline to AGENTS.md. Co-authored-by: Ante Barić <capJavert@users.noreply.github.com>
__tests__/workers/cdc/primary.ts
Outdated
| embedding: undefined, | ||
| enriched: undefined, | ||
| language: undefined, | ||
| scraped: undefined, |
There was a problem hiding this comment.
I don't know why it added these, undefined is not needed and tests pass without it
There was a problem hiding this comment.
@claude remove changes to this file, they are not needed
There was a problem hiding this comment.
Claude finished @capJavert's task in 55s —— View job
Reverting Unnecessary Test Changes
Tasks:
- Read the file to see what changes were made
- Check git diff to identify the specific changes
- Revert the unnecessary changes to the test file
- Commit and push changes
Summary
Reverted the changes to __tests__/workers/cdc/primary.ts that added explicit undefined field assignments to test expectations. These fields don't need to be explicitly set to undefined in test assertions - Jest/TypeScript handle optional fields correctly without them.
Changes:
- Removed 28 lines of explicit
undefinedassignments from test expectations - Test expectations now only include the fields that actually have values
Commit: 98004ae
--- | Branch
Co-authored-by: Chris Bongers <rebelchris@users.noreply.github.com>
Remove explicit undefined field assignments from test expectations Co-authored-by: Ante Barić <capJavert@users.noreply.github.com>
…itch Inline the standalone function directly into the handler to remove unnecessary indirection, and replace nested ternary with switch statement. Add inline handler best practice to workers AGENTS.md. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
capJavert
left a comment
There was a problem hiding this comment.
If tests pass it should be good
a2fac62 to
6db1b5f
Compare
|
@claude read all the comment threads from @capJavert from today (2026-02-13) and reaply all the changes you made Some dumb human might have rebased and force pushed without pulling the latest changes |
|
Claude finished @davidercruz's task in 3m 24s —— View job Re-applying Lost Changes from Review CommentsI've successfully re-applied all the changes that were made in response to @capJavert's review comments from today (2026-02-13). Tasks:
Changes Re-applied1. Zod Type Inference (
2. Worker Error Handling (
3. Logging Cleanup (
4. Test Cleanup (
5. Documentation Updates (
Commit: |
bdb6bf4 to
a2fac62
Compare
This adds Bragi rejection classification to the opportunity feedback worker while keeping existing per-item feedback parsing. It stores structured rejection reasons and summary on OpportunityMatch via a new JSONB column and migration. The Bragi schema dependency is bumped to 0.2.70 to use the typed classifyRejectionFeedback RPC. Tests now cover storing rejection classification, idempotent skip when already present, and graceful handling of ConnectError.