Feature/dynamic criteria extra fields - #14
Open
borkarsaish65 wants to merge 15 commits into
Open
Conversation
…coded Replaces the hardcoded EXTRA_KEYS/ENROLLMENT_TASK_FILTER mechanism (one fixed task, three fixed fields, ~90 lines of duplicated prompt text per evidence type) with a generic one driven by optional extraction_field/ extraction_description/extraction_type columns in the criteria CSV. Any task can now define any number of extra fields to extract from evidence, with the LLM prompt built dynamically from each field's plain-English description instead of hand-written per-field instructions baked into the script. Drops the enrollment-specific sanity-check validation (validate_and_fix_enrollment_data) in favor of plain type casting, since the range/cross-field heuristics don't generalize to arbitrary fields. Co-Authored-By: Claude Sonnet 5 <[email protected]>
Adds upload-time validation for the field_name/field_description/value_type columns in the criteria CSV: rejects a row that defines field_name but has an unsupported value_type or blank field_description, before any AI cost is spent. Previously a bad row only surfaced as a silent warning deep inside the processor script mid-execution. Also renames the extraction columns (extraction_field/extraction_description/ extraction_type -> field_name/field_description/value_type) for readability, after an earlier iteration through a single-JSON-array-column format proved less friendly for non-technical CSV authors than plain columns. Co-Authored-By: Claude Sonnet 5 <[email protected]>
Adds the enrollment task's 3 extraction rows (Enrollment_2024, Enrollment_2025, Enrollment_Increase_Percentage) to sample_criteria.csv, the file bootstrap.py uploads to cloud storage on every service startup. Without this the deployed sample would ship with no extraction fields configured at all, giving no working example of the feature. Co-Authored-By: Claude Sonnet 5 <[email protected]>
…a CSV Adds a Student_Count extraction field to Q-102 (group discussion criterion), mirroring the projects vertical's sample_criteria.csv update, so this shipped sample also has a working example of the feature instead of only the projects one. Co-Authored-By: Claude Sonnet 5 <[email protected]>
…ort/observation services/bootstrap.py::_upload_sample_csvs skips uploading a sample CSV once its DB field is populated from a prior startup, regardless of whether the local repo file has since changed. The projects and observation sample_criteria.csv files were updated to demonstrate the new extraction fields, but the stale pre-update content was still what any existing deployment's bootstrap had already pushed to cloud storage. This resets sample_criteria_file_url to NULL for just those two default-scope rows so the next startup treats them as never-uploaded and pushes the current file. Verified: ran scripts/upload_sample_csvs.py after applying this migration and confirmed cloud storage now serves the updated CSV content for both project_report and observation. Co-Authored-By: Claude Sonnet 5 <[email protected]>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
…gration The previous downgrade() was a no-op pass, incorrectly reasoning that the cleared sample_criteria_file_url values couldn't be reconstructed. They can: the value is always "/" + the fixed cloud_path string from _SAMPLE_UPLOAD_MANIFEST in services/bootstrap.py, identical for every deployment, not runtime-generated data. downgrade() now restores that deterministic path, guarded by "IS NULL" so it only fixes rows this migration's own upgrade() cleared rather than rows that are NULL because bootstrap simply hasn't run yet. Verified with a live round-trip: upgrade clears to NULL, downgrade restores /projects/sample_criteria.csv and /observation/sample_criteria.csv exactly, upgrade clears again. Co-Authored-By: Claude Sonnet 5 <[email protected]>
A row that set field_name but left its task/context column blank passed upload validation (only value_type/field_description were checked), then silently produced no data: pandas reads a blank CSV cell as NaN, and str(NaN).strip() is the literal text "nan", not an empty string, so load_questions_mapping()'s "if has_extraction_columns and norm_key" gate doesn't filter it out — it registers the field under the bogus task key "nan", which no real evidence row ever matches. The output column exists but is blank for every row, discovered only after a full paid AI run. Now rejected at upload validation with a message naming the row and field. Co-Authored-By: Claude Sonnet 5 <[email protected]>
… migration Was hardcoded to literal 'default'/'default_code', but core/config.py's DEFAULT_TENANT_CODE and DEFAULT_ORGANIZATION_CODE are env-var-overridable — a deployment that customized them would have its CsvSourceType rows under a different (tenant_code, organization_code), so the migration would match zero rows and silently leave that deployment's sample criteria file stale. _resolve_scope() now mirrors services/bootstrap.py::_resolve_scope() exactly (same env var, same fallback), and both upgrade()/downgrade() use sa.text().bindparams() instead of literal strings in the raw SQL. Verified with a full 4-step round-trip against the live DB (no-op when already populated, clear to NULL, restore, clear again) — all correct. Co-Authored-By: Claude Sonnet 5 <[email protected]>
The previous downgrade() restored a deterministic sample-file URL for any row with sample_criteria_file_url IS NULL, on the assumption that NULL only ever meant "this migration's upgrade() cleared it." That's not true: a row is also NULL when bootstrap has never successfully uploaded the file yet (fresh environment, or a prior upload attempt that failed) — the two cases are indistinguishable from the column's current value alone. Stamping in a URL for a row in the second case would make services/bootstrap.py::_upload_sample_csvs skip it permanently (its "already uploaded" check just looks at whether the field is populated), while the object genuinely doesn't exist in cloud storage — every signed download URL from that row would 404, with no future startup able to self-heal it. downgrade() is now an explicit no-op: it can't corrupt state, where a wrong guess could. This migration exists to fix one specific, narrow, known data issue on this deployment, not as a reusable general-purpose migration, so a documented no-op is the correct tradeoff here rather than adding a backup-table snapshot mechanism to make a "real" restore actually safe. Verified with a live round-trip: downgrading from a populated state leaves it untouched; downgrading from NULL correctly stays NULL rather than fabricating a URL; upgrade still clears to NULL as before. Co-Authored-By: Claude Sonnet 5 <[email protected]>
priyanka-TL
requested changes
Aug 4, 2026
_validate_extraction_fields_column treated a blank task_column parameter (evidence_context_config.criteria_csv_column and input_csv_column both unset for the source type — e.g. the minimal_config_test fixture) the same as a blank cell in a configured column, rejecting every row with a nonsensical "the '' column must not be blank" error even when the criteria file's actual task values were perfectly valid. Now only enforces the task-column-non-blank check when a task column is actually configured; when none is configured, that check is skipped entirely rather than failing on an empty column name. Co-Authored-By: Claude Sonnet 5 <[email protected]>
field_name comes straight from the user's criteria CSV and gets written directly as a column of the output CSV. The processor's stub-creation step guards against overwriting an existing column (1-main-parallel-script.py: 1946-1948), but the steps that actually write real data (_flush_to_csv, the end-of-run rebuild) assign unconditionally. A field_name of e.g. "Relevance Tag" or the tenant's own school-id column silently overwrites that column's real values for the entire run, with report_service.py then reading corrupted data downstream and nothing telling the user it happened. _validate_extraction_fields_column now rejects any field_name matching either the 7 fixed derived output columns (new RESERVED_OUTPUT_COLUMNS constant) or the input CSV's own headers (passed in from input_result.columns_detected, already computed earlier in validate_execution_files for the input file's own validation). Verified against 5 collision/no-collision cases plus a full regression pass over every known-good criteria CSV using realistic input columns — no false rejections, real collisions correctly caught. Co-Authored-By: Claude Sonnet 5 <[email protected]>
_validate_extraction_fields_column rejected any value_type not exactly
int/float/string, including blank. But the processor already treats a
blank value_type leniently — str(row.get("value_type", "")).strip().lower()
or "string" in 1-main-parallel-script.py:840 — silently defaulting to
string. So a criteria CSV that would have run fine before this PR was now
rejected at validation purely for leaving value_type blank, a real
regression for existing users with no functional upside: an explicit typo
like "integer" is still caught, only blank is now allowed through.
Co-Authored-By: Claude Sonnet 5 <[email protected]>
Adds a Material_Type example (Q-101, string type) alongside the existing Student_Count one (Q-102, int type), and appends a bracketed note to both field_description values explaining the concrete effect of that row: which output column it creates and what fills it in, per PR ELEVATE-Project#14 review feedback (priyanka-TL) requesting a row that explains what actually happens as a result of the configuration, not just what to type. Co-Authored-By: Claude Sonnet 5 <[email protected]>
…ration Per PR ELEVATE-Project#14 review feedback (priyanka-TL): there was no way to update CsvSourceType config at runtime, only a raw SQL migration for this one-time fix. Replaces the migration with a real, reusable capability: - PATCH /config/csv-source-types/{type_key} — partial update, only fields present in the request body are changed (Pydantic model_dump(exclude_unset) distinguishes "explicitly sent" from "omitted", so {"sample_criteria_file_url": null} clears just that field without touching anything else). - Superuser-only (current_user.is_superuser) — the first place in this codebase that actually enforces that flag; it existed on the User model and was correctly seeded, but nothing ever checked it. Added is_superuser to UserResponse so current_user actually exposes it. - Scoped to the caller's own tenant/organization via ConfigService._resolve_scope(), matching the existing pattern. Removed the ddcb170ce4b3 migration entirely rather than keeping both paths: a fresh install never needs it (bootstrap uploads the already-corrected sample CSVs on its first run, automatically), so the migration only ever mattered for environments that already bootstrapped with the stale content before this fix — a small, known set that the new API can fix directly, without carrying a single-purpose migration in the schema history forever. Verified live end-to-end against the running app with real accounts: logged in as admin (is_superuser=true), PATCHed both project_report and observation to clear sample_criteria_file_url (confirmed via direct DB read that only that field changed, sample_input_file_url untouched), confirmed program_designer (is_superuser=false) gets 403, then ran scripts/upload_sample_csvs.py and confirmed it actually re-uploaded both files instead of skipping. Co-Authored-By: Claude Sonnet 5 <[email protected]>
priyanka-TL
requested changes
Aug 5, 2026
priyanka-TL
left a comment
priyanka-TL
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed on Aug 5th 11.20 AM
| the whole run with extraction data instead, corrupting report_service.py's | ||
| downstream analytics with no indication anything went wrong. | ||
| """ | ||
| if "field_name" not in headers: |
Collaborator
There was a problem hiding this comment.
@borkarsaish65
Extraction fields are ignored if any required companion column is missing.
Currently, upload validation only checks for the field_name column, so the criteria file is accepted even when the other required extraction columns are missing. During processing, extraction only runs if all required columns are present, so the requested extraction fields are silently skipped. This can cause users to run a full analysis expecting extracted fields, but the report is generated without them and without any validation error.
| email: str | ||
| full_name: Optional[str] = None | ||
| is_active: bool | ||
| is_superuser: bool = False |
Collaborator
There was a problem hiding this comment.
@borkarsaish65 why this is required ? To update the config as of now use the internal-access-token in header. Validate that with env variable
| detail="No fields provided to update.", | ||
| ) | ||
|
|
||
| for field_name, value in changed_fields.items(): |
Collaborator
There was a problem hiding this comment.
@borkarsaish65 Check devin comment
Clearing a required configuration value returns an unexplained server error
The configuration update writes whatever the caller sent straight onto the record (setattr(source_type, field_name, value) at config_service.py:318-319) with no check that the value is allowed to be empty, so clearing a mandatory setting fails at save time with a generic internal error. Impact: An administrator clearing a required setting sees a 500 internal error containing raw database text instead of a clear "this field cannot be empty" message.
| f["field"] for fields in _main_extra_fields_by_task.values() for f in fields | ||
| }) | ||
| if all_extra_field_names: | ||
| logging.info(f"[Main] Extra field extraction ENABLED. Fields to extract:") |
Collaborator
There was a problem hiding this comment.
@borkarsaish65 New log statements use string interpolation instead of the required formatting style
Several newly added log lines build their text with f-strings (logging.info(f"[Main] Extra field extraction ENABLED. Fields to extract:") at 1-main-parallel-script.py:2419), which the repository's coding standards explicitly forbid. Impact: The project's logging convention is broken, making machine parsing of log output inconsistent.
| self._validate_questions_csv_metadata(headers, source_type) | ||
|
|
||
| @classmethod | ||
| def _validate_extraction_fields_column( |
Collaborator
There was a problem hiding this comment.
@borkarsaish65 _validate_extraction_fields_column does not check that the referenced task also has a criteria row somewhere in the file;
| questions_result.columns_detected = headers | ||
| questions_result.preview_rows = preview_rows | ||
| self._validate_questions_csv_metadata(headers, source_type) | ||
| self._validate_extraction_fields_column( |
Collaborator
There was a problem hiding this comment.
@borkarsaish65 check devin comment
Reserved-column collision check is skipped when the input file fails to validate
_validate_extraction_fields_column is passed input_result.columns_detected, which is only populated after the input CSV downloads and parses successfully (execution_service.py:1685). If the input file is missing, undownloadable, or fails parsing, the collision set falls back to just RESERVED_OUTPUT_COLUMNS, so a field_name matching a real input column (e.g. School ID) is accepted. The user then fixes the input file, re-validates only... actually re-validation runs both files again, so the collision is caught on the successful pass — but if the user re-uploads only the input file and starts a run without re-validating, the checkpoint keeps the earlier "questions validated" state. Worth confirming that start_execution always requires a validation pass in which both files succeeded.
- Reject a criteria CSV with 'field_name' but no 'field_description'/ 'value_type' header — the processor's has_extraction_columns gate silently disables extraction file-wide otherwise, not just per-row. - Reject an extraction field whose task has no row with a non-blank question anywhere in the file — such a task never gets a mapped question stamped by the pre-processor, so every one of its rows is silently skipped as User-Owned, extraction fields included. - Replace the is_superuser gate on PATCH /config/csv-source-types with an X-Internal-Access-Token header checked against a new required INTERNAL_ACCESS_TOKEN env var; JWT auth stays for tenant/org scoping and the updated_by audit trail. - Reject clearing a NOT NULL CsvSourceType column via that same PATCH endpoint with a clean 422 instead of a raw DB IntegrityError; give it its own REQUIRED_FIELD_EMPTY error code distinct from the empty-body 400 case, and wrap the token-check 403 in the standard response envelope by calling it from inside the route handler's try/except instead of as a bypassing Depends(). - Convert f-string log calls added for this feature to %s formatting, matching the repo's logging convention. Co-Authored-By: Claude Sonnet 5 <[email protected]>
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
2 participants
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.
No description provided.