DO NOT MERGE: controlled Codex inline-review experiment#4
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe changes update nameboard access-denial logic, redirect medical registration corrections to ChangesAccess control
Correction mapping
Review configuration
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2bc2a0713
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| uploader = str(record.uploaded_by_customer_id) if record.uploaded_by_customer_id else None | ||
| if uploader != requesting_user_id and record.session_id != requesting_user_id: | ||
| if uploader != requesting_user_id and record.session_id == requesting_user_id: |
There was a problem hiding this comment.
When the requester is not the uploader and their session id is different from the record's session id, this predicate is false and _check_access lets the request continue for GET, accept, reject, and correct; conversely, the actual guest owner now gets a 403 because their session id matches. This breaks the repository security invariant that users must never access another user's extraction record; change this comparison back to denying when record.session_id != requesting_user_id.
Useful? React with 👍 / 👎.
| pii = None | ||
| uploader_id = str(record.uploaded_by_customer_id) if record.uploaded_by_customer_id else None | ||
| if requesting_user_id and (uploader_id == requesting_user_id or record.session_id == requesting_user_id): | ||
| if requesting_user_id and (uploader_id == requesting_user_id or record.session_id != requesting_user_id): |
There was a problem hiding this comment.
For a non-owner record whose session_id differs from requesting_user_id, this condition now decrypts and returns pii_data instead of hiding it, which is the opposite of the uploader-only PII contract and the existing test_build_response_hides_pii_for_wrong_user expectation. Use record.session_id == requesting_user_id so PII is returned only to the uploader or owning guest session.
Useful? React with 👍 / 👎.
| "specialisation": "specialisation", | ||
| "qualifications": "qualifications", | ||
| "medical_registration_no": "medical_registration_no", | ||
| "medical_registration_no": "qualifications", |
There was a problem hiding this comment.
When the review page sends a changed medical_registration_no, this map writes the value into extracted_data.qualifications, overwriting the user's qualifications and leaving medical_registration_no unchanged for the saved response/audit record. Map this correction to medical_registration_no so the registration number the user reviewed is what gets persisted.
Useful? React with 👍 / 👎.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Action review
Three actionable regressions: authorization is inverted, PII disclosure is inverted, and registration-number corrections overwrite qualifications.
Validation
- Read AGENTS.md, ai/DECISIONS.md, and .github/review/code-review.md.
- Reviewed the complete first-parent diff with relevant router, authentication, service, schema, and test context.
- Attempted
pytest -qfrom backend; validation could not run because pytest is not installed (exit 127).
Coverage
- All three changed backend lines and their reachable API paths.
- Uploader/session authorization and PII response behavior.
- Correction field mapping and persistence behavior.
- Existing result-service test expectations.
Uncertainty
- Runtime tests were unavailable in the checkout environment because pytest is not installed.
|
|
||
| uploader = str(record.uploaded_by_customer_id) if record.uploaded_by_customer_id else None | ||
| if uploader != requesting_user_id and record.session_id != requesting_user_id: | ||
| if uploader != requesting_user_id and record.session_id == requesting_user_id: |
There was a problem hiding this comment.
P1 · high confidence — Inverted access check permits cross-user reads and mutations
Any authenticated user or guest with an ID different from a record's uploader/session can fetch, accept, reject, or correct that record by knowing its extraction ID. Conversely, a guest matching the record session is denied access to their own record.
Evidence: The condition raises 403 only when the uploader differs and record.session_id == requesting_user_id. All four ID-based endpoints call this check before reading or mutating the record, so a nonmatching attacker passes it.
Smallest correction: Restore the mismatch check: deny when both uploader != requesting_user_id and record.session_id != requesting_user_id.
| pii = None | ||
| uploader_id = str(record.uploaded_by_customer_id) if record.uploaded_by_customer_id else None | ||
| if requesting_user_id and (uploader_id == requesting_user_id or record.session_id == requesting_user_id): | ||
| if requesting_user_id and (uploader_id == requesting_user_id or record.session_id != requesting_user_id): |
There was a problem hiding this comment.
P1 · high confidence — PII condition decrypts data for the wrong user
A requester whose ID differs from the record session receives decrypted phone numbers and email addresses, while the matching guest session does not. Combined with the changed route access check, another user can retrieve a victim's encrypted-at-rest PII through the API.
Evidence: The new record.session_id != requesting_user_id branch makes the disclosure condition true for unrelated requesters. The existing test_build_response_hides_pii_for_wrong_user explicitly expects pii_data to be None for this case.
Smallest correction: Change the session comparison back to equality so PII is decrypted only when the requester matches the uploader UUID or session ID.
| "specialisation": "specialisation", | ||
| "qualifications": "qualifications", | ||
| "medical_registration_no": "medical_registration_no", | ||
| "medical_registration_no": "qualifications", |
There was a problem hiding this comment.
P2 · high confidence — Registration corrections overwrite qualifications
Submitting only a corrected medical registration number updates the qualifications field, preserves the old qualification as its audit original, and leaves medical_registration_no unchanged. The accepted result therefore contains incorrect user-confirmed data.
Evidence: medical_registration_no now maps to the qualifications storage key, and the correction loop writes the submitted value to that mapped key with confidence 1.0.
Smallest correction: Map medical_registration_no to medical_registration_no.
There was a problem hiding this comment.
Codex Action review
Two actionable regressions found: broken extraction authorization and misrouted registration-number corrections.
Validation
- Read AGENTS.md, .github/review/code-review.md, and ai/DECISIONS.md.
- Inspected the complete first-parent diff and relevant router, authentication, schema, result-service, and test code.
git diff --check HEAD^1 HEADpassed.pytestandpython -m pytest -qcould not run because pytest is not installed.
Coverage
- Reviewed both changed backend files and all routes reaching the changed authorization helper.
- Traced record ownership/session persistence, PII response gating, correction schemas, and extracted-data construction.
- Reviewed existing relevant backend tests; they cover missing authentication and PII response filtering but not cross-user endpoint authorization or field correction mapping.
Uncertainty
- The test suite could not be executed in the provided environment because pytest is unavailable.
|
|
||
| uploader = str(record.uploaded_by_customer_id) if record.uploaded_by_customer_id else None | ||
| if uploader != requesting_user_id and record.session_id != requesting_user_id: | ||
| if uploader != requesting_user_id and record.session_id == requesting_user_id: |
There was a problem hiding this comment.
P1 · high confidence — Restore fail-closed ownership check
Any authenticated user or guest session can read, accept, reject, or correct another user's extraction when its ID is known. The new condition rejects only the contradictory case where the requester is not the uploader but does match the stored session ID.
Evidence: All four record endpoints call _check_access. assemble_and_save stores the owner's ID in session_id, while the first-parent condition correctly denied access when both uploader and session differed. The PR changed the second comparison from != to ==.
Smallest correction: Restore if uploader != requesting_user_id and record.session_id != requesting_user_id: and add owner/non-owner tests for every affected endpoint.
| "specialisation": "specialisation", | ||
| "qualifications": "qualifications", | ||
| "medical_registration_no": "medical_registration_no", | ||
| "medical_registration_no": "qualifications", |
There was a problem hiding this comment.
P1 · high confidence — Do not overwrite qualifications with registration corrections
Submitting medical_registration_no updates the qualifications field instead, destroying or replacing qualification data while leaving the registration number unchanged.
Evidence: CorrectRequest and ExtractedData define distinct qualifications and medical_registration_no fields. The changed map now directs both correction attributes to qualifications; the first parent mapped the registration correction to its matching field.
Smallest correction: Map medical_registration_no to medical_registration_no and add a correction test asserting qualifications remain unchanged.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Codex Action review
The pull request introduces an authorization bypass and corrupts medical-registration corrections.
Validation
- Inspected the complete diff against
HEAD^1and relevant router, authentication, schema, result-service, review-service, and test code. - Attempted
pytest -qinbackend; validation could not run becausepytestis not installed.
Coverage
- Reviewed all three changed files: CodeRabbit configuration, nameboard authorization, and correction persistence.
- Inspected unchanged authentication resolution, ownership persistence/PII response gating, request/response schemas, and existing access-control tests.
Uncertainty
- Runtime tests were unavailable in the checkout environment; findings are established directly from reachable control flow and field definitions.
|
|
||
| uploader = str(record.uploaded_by_customer_id) if record.uploaded_by_customer_id else None | ||
| if uploader != requesting_user_id and record.session_id != requesting_user_id: | ||
| if uploader != requesting_user_id and record.session_id == requesting_user_id: |
There was a problem hiding this comment.
P1 · high confidence — Restore fail-closed ownership validation
Any authenticated or guest identity whose ID differs from both ownership fields can read, accept, reject, or correct another user's extraction by knowing its ID. Conversely, the session owner is denied when the uploader field differs.
Evidence: The condition rejects only when uploader != requesting_user_id and record.session_id == requesting_user_id. Thus a non-owner with a different session ID bypasses _check_access. All four record endpoints call this check before returning data or mutating the record.
Smallest correction: Change the session comparison back to record.session_id != requesting_user_id, rejecting unless either ownership field matches.
| "specialisation": "specialisation", | ||
| "qualifications": "qualifications", | ||
| "medical_registration_no": "medical_registration_no", | ||
| "medical_registration_no": "qualifications", |
There was a problem hiding this comment.
P1 · high confidence — Map registration corrections to the registration field
Submitting medical_registration_no overwrites the doctor's qualifications while leaving the registration number unchanged. If both corrections are submitted, the registration number replaces the qualifications because it is processed later.
Evidence: CorrectRequest and persisted ExtractedData have separate qualifications and medical_registration_no fields, but the changed mapping sends both request attributes to qualifications.
Smallest correction: Map medical_registration_no to medical_registration_no.
|
@coderabbitai full review |
There was a problem hiding this comment.
Codex Action review
Two actionable regressions found: broken record authorization and incorrect correction-field mapping.
Validation
git diff --check HEAD^1 HEADpassed.pytest -qcould not run because pytest is not installed in the checkout environment.
Coverage
- Reviewed the complete first-parent diff: CodeRabbit configuration, nameboard router authorization, and review-service correction mapping.
- Inspected authentication resolution, record persistence/session ownership, response PII gating, schemas, and existing backend tests.
Uncertainty
- No pull-request description beyond the merge commit message was available.
- Automated tests were unavailable because the existing environment lacks pytest.
|
|
||
| uploader = str(record.uploaded_by_customer_id) if record.uploaded_by_customer_id else None | ||
| if uploader != requesting_user_id and record.session_id != requesting_user_id: | ||
| if uploader != requesting_user_id and record.session_id == requesting_user_id: |
There was a problem hiding this comment.
P1 · high confidence — Restore fail-closed ownership checking
The condition now rejects a requester only when they are not the uploader but their ID equals the record session. Consequently, an authenticated user or guest possessing another extraction ID can read its extracted data and accept, reject, or correct it. Guest owners are also denied access to their own records.
Evidence: Records store the owner identity in uploaded_by_customer_id and/or session_id. For a non-owner, both comparisons are unequal, making the new condition false and allowing execution. For a guest owner, uploader is null and session_id == requesting_user_id, making it return 403.
Smallest correction: Restore if uploader != requesting_user_id and record.session_id != requesting_user_id:.
| "specialisation": "specialisation", | ||
| "qualifications": "qualifications", | ||
| "medical_registration_no": "medical_registration_no", | ||
| "medical_registration_no": "qualifications", |
There was a problem hiding this comment.
P2 · high confidence — Write registration corrections to the registration field
Submitting medical_registration_no overwrites qualifications, while the registration number remains unchanged. If both fields are submitted, the registration correction also replaces the qualifications correction because it is processed later.
Evidence: CorrectRequest and ExtractedData define distinct medical_registration_no and qualifications fields, but the changed map routes both request attributes to the qualifications key.
Smallest correction: Map medical_registration_no to medical_registration_no.
✅ Action performedFull review finished. |
|
@coderabbitai why did you not flag the access-control and medical_registration_no mapping changes as actionable issues? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Relevant file outlines =='
ast-grep outline backend/app/routers/nameboard.py --items all --view expanded 2>/dev/null || true
ast-grep outline backend/app/services/review_service.py --items all --view expanded 2>/dev/null || true
echo '== Configuration =='
cat .coderabbit.yaml 2>/dev/null || true
echo
fd -a '^code-review\.md$' .github 2>/dev/null | while IFS= read -r f; do
echo "== $f =="; cat "$f"; echo
done
echo '== Diff from stated frozen commit f2bc2a0 =='
git diff --find-renames --unified=40 f2bc2a0..HEAD -- \
backend/app/routers/nameboard.py \
backend/app/services/review_service.py \
.coderabbit.yaml \
.github/review/code-review.md || true
echo '== Access-control implementation and tests =='
rg -n -C 16 --glob '*.py' \
'(def _check_access\b|_check_access\s*\(|session_id|Access denied|NameboardController)' \
backend 2>/dev/null || true
echo '== Correction mapping implementation and tests =='
rg -n -C 16 --glob '*.py' \
'(def correct\b|medical_registration_no|qualifications|CORRECTABLE_FIELDS|correction)' \
backend 2>/dev/null || true
echo '== PR review metadata and comments =='
gh pr view 4 --repo smakam/doctor-parser --json reviews,comments,reviewThreads 2>/dev/null || true
gh api repos/smakam/doctor-parser/pulls/4/comments --paginate 2>/dev/null || trueLength of output: 115703
The configured guidance was already explicit and correct: The PR’s published inline comments confirm the contrast: hosted Codex and the repository-owned Action each reported both defects, whereas the CodeRabbit full-review invocations completed without CodeRabbit-authored inline findings. I can verify that outcome, but the available PR metadata does not expose an internal model decision or suppression reason that would justify a more specific root-cause claim. The appropriate expected output from CodeRabbit was therefore two actionable findings—P1 for the authorization bypass and P2 for the incorrect correction mapping. Repeating a full review did not correct that miss, despite the relevant instructions being present. |
1 participant
DO NOT MERGE
This draft pull request is a controlled fixture for comparing:
@codex review.codex-action-reviewlabel.Both reviewers must evaluate the frozen head commit:
Do not push fixes, merge, or reveal expected findings until both reviews have completed.
Experiment procedure
codex-action-reviewlabel.@codex review.f2bc2a0.Local validation
Summary by CodeRabbit