# Final whole-branch review — undergraduate integration
Executor resolution (2026-10-15): all three Important findings and the record-type rendering failure (regraded Important) were reproduced RED then fixed GREEN; final Flutter suite249/249. Original independent report retained below. Live SSO remains unverified; all artifact checks and decisions are in tpschedule-0.7.0-validation.md.
Date: 2026-10-15. This resumes the interrupted final review; it is not a second review or fix pass. Source, index, HEAD and branch state were not changed by this reviewer. Only this requested ignored review artifact was written inside the checkout. Temporary probes used the sandbox temp directory.
## Strengths
- Undergraduate imports have a separate connector, explicit manual timetable capture, and an all-types exam merge; the graduate workflow remains separately dispatched.
- School-qualified terms, stable persisted school IDs, period-12 source validation, per-page byte limits, narrow navigation origins, and inherited atomic persistence are coherent safeguards.
- Timetable cancellation subtraction and arrangement grouping preserve teachers, rooms and weeks. Undergraduate exam imports preserve the current timetable and date settings. Source labels and 11/12-row selection use saved data.
- The implementation includes focused parser, session, controller, widget and service regressions. The executor reports 241 Flutter tests, analysis and both Node probes green, native sample counts of 18 timetable arrangements and 4 examinations, and Android offline-flow checks. These are executor evidence, not independently rerun suite results in this review.
## Issues
### Critical
None identified in the reviewed range.
### Important
1.**Invalidate completed exam results when the browser starts a new document.**
- Location: `mobile/lib/features/school/undergraduate/session.dart:110–115`; related save guard at `mobile/lib/features/school/undergraduate/auth_page.dart:64–70`.
-`onPageStarted` clears only `pendingDraft`; in exam mode it leaves both `pendingExams` and `_generation` unchanged. `_complete` also stays true, so subsequent `_loaded` callbacks return before detecting an authentication change.
- Reproduction: finish an exam batch, then have a navigation to a different document (for example an authentication redirect) start before the deferred/queued save executes, or navigate after a failed save while the result remains pending. The captured generation and pending result identity still match. The page's `canApply` guard therefore accepts the obsolete exam result, and `session.saved(generation: oldGeneration)` also returns true. A fresh navigation or account transition cannot invalidate this completed result as required by the spec.
- Fix: separate document validity from intentional within-batch navigation, or explicitly invalidate completed/pending results on subsequent document starts; include document validity in the save guard. Keep normal type-to-type navigation working. Add a regression with a completed result, blocked controller queue, then new navigation before queue release.
- Evidence: direct control-flow inspection. A temporary external Flutter reproduction was prepared but did not execute assertions because the sandbox denied copying the compiled test artifact; no runtime pass is claimed for this issue.
2.**Preserve independently saved undergraduate exams when importing a new graduate timetable term.**
- The `clearExams` predicate checks the incoming timetable's school, but never checks the saved exam schedule's school. Whenever a graduate timetable changes term, it deletes the saved exam schedule even if that schedule is undergraduate data intentionally independent of the timetable.
- Reproduction: start with graduate timetable term 73; successfully import undergraduate exams for term 503; import graduate timetable term 72. `_differentTerm` is true and the incoming school is graduate, so `copyWith(clearExams: true)` persists removal of the undergraduate results and their successful query choices. The user requested a timetable update, not deletion of the independent exam query.
- Fix: preserve undergraduate exam schedules regardless of incoming timetable school; retain legacy graduate clearing only for graduate exam schedules to which that rule applies. Add this direction to the existing controller test, which currently tests only importing an undergraduate timetable after saving undergraduate exams.
- Evidence: direct evaluation of the persistence predicate and `copyWith` path. Same external Flutter test-run limitation as issue 1.
3.**Reject unconsumed activity/index syntax instead of silently importing a partial timetable.**
- Location: `mobile/lib/features/school/undergraduate/activity_data.dart:48–54` (also activity matching at lines 20–24).
- Completeness is measured by counting only already-recognized `index = ...;` and `tableN.activities[index][...] = activity;` statements. Unsupported additional activity writes are not included in either count, so they can silently disappear while the recognized subset passes validation. This violates the explicit unknown-index rejection and old-data-preservation requirement.
- Minimal reproduction: use a valid TaskActivity and the usual assignment at index `0*unitCount+0`, then append `table0.activities[1*unitCount+1][table0.activities[1*unitCount+1].length] = activity;`. The parser returns one activity with indices `{0}` and no error, dropping the Tuesday lesson. The timetable adapter has no subsequent full-script consumption validation; such a successful draft can replace a complete stored timetable and change reminders.
- Fix: tokenize/validate the complete relevant activity block, or detect every TaskActivity declaration and activities write and reject any unconsumed form. Supporting arbitrary JavaScript is unnecessary; rejecting unsupported syntax is sufficient. Cover a recognized statement followed by an unrecognized extra assignment, not only an unknown expression inside a recognized `index =` assignment.
- Evidence: an independent temporary Dart probe against the actual `parseActivityData` implementation ran successfully and printed `Parsed activities: 1; indices: {0}; direct second assignment silently omitted`. No private student data was used.
### Minor
4.**Validate the optional record-level exam type before accepting stored graduate records.**
- Location: `mobile/lib/domain/exam_schedule.dart:61` and `:116–120`; consumer `mobile/lib/features/exams/exams_page.dart:190` in the reviewed revision.
-`ExamRecord.fromJson` accepts any integer `examTypeId`. The schedule-level membership check runs only for undergraduate schedules, while the examination page calls `UndergraduateExamType.fromId` for every non-null record type, regardless of school.
- Reproduction: deserialize a graduate/default-school schedule with otherwise valid record fields and `examTypeId: 99`. Loading succeeds; opening its examination card throws `FormatException: 考试类型无效`. A damaged optional field therefore bypasses the intended invalid-save/backup recovery path and instead crashes the page rendering. This is lower likelihood because the current graduate adapter does not emit this field.
- Fix: reject non-null type IDs outside 1–3 at the model boundary, and/or require the field to be absent for graduate data. Keep old records with no field compatible.
- Evidence: independent Dart probe loaded type 99 successfully and reproduced the enum conversion exception used by the page.
## Verification and limitations
- Read the production diff, spec, plan, progress ledger, relevant existing tests, and validation record. Inspected the exact code paths above.
- Two pure-Dart reproduction cases ran against the implementation: unsupported activity assignment omission and invalid record-type acceptance/render conversion.
- Three additional external temporary Flutter probes were attempted for the controller, completed-session invalidation and full timetable adapter. Flutter failed before assertions with an output.dill/listener.dart.dill copy permission error in the sandbox temp directory. The process was stopped. This is a test-environment limitation, not a product test failure, and those probes are not counted as executed regressions.
- The semicolon-omission parser candidate was considered and rejected as a finding: the tested form threw a FormatException rather than silently importing. The confirmed parser finding above uses an unrecognized extra assignment instead.
## Recommendations
Fix issues 1–3 before shipping, adding narrow regressions for their triggers. Issue 4 is a small model-boundary fix consistent with the corrupt-data requirement. Run the relevant session/controller/parser/model tests and the usual final checks after fixes. Rebuild artifacts if shipped source changes. No second independent review pass is requested by this report.
## Declined to judge
- Actual undergraduate SSO, CAPTCHA and live school callback behavior: no authenticated school session was available; offline fixtures and fake WebView callbacks cannot establish successful real authentication. This remains a documented release validation boundary, not a claimed pass.
- Query-label/card spacing and the glass offset/shader changes made after `674df1c`: explicitly separate current user requests, outside this fixed review range, being handled by the executor.
- Frozen registration materials, private signing keys and current APK signature/ZIP contents: this is the requested source-branch safety review; artifact verification is recorded by the executor. No private key or original student HTML was opened or printed by this reviewer.
## Assessment
**Ready to merge: With fixes.**
The core separation and persistence design follows the plan, but document invalidation, cross-school exam retention and fail-closed activity parsing have reproducible gaps that can save obsolete data or remove valid data. Resolve the three Important findings before delivering the updated build; the Minor finding is also straightforward to address.
Executor resolution (2026-10-15): structured static failure causes and numeric HTTP/WebView status are now recorded; graduate probe exceptions produce a type-only breadcrumb. Login diagnostic regression was observed RED→GREEN; final Flutter suite249/249 and backend54/54. Original independent report follows.
Scope: new diagnostic logging and feedback multipart changes only, compared with HEAD. Undergraduate final-review fixes and glass changes were excluded. Read-only inspection; no deployment or private page content accessed. Existing test additions inspected; no redundant test run while the implementer is running the feature suite.
## Important finding
[P2] Preserve a structured reason for terminal school-read failures.
Location: `mobile/lib/features/school/school_session.dart:272-278` (and equivalent `mobile/lib/features/school/undergraduate/session.dart:219-225`). All callers currently produce `school_read_failed` with the same `code: flow_failure`. Minimal trigger: let the graduate main-frame HTTP request fail versus let the same target time out; both logs contain the identical failure event even though the UI already distinguishes those causes. A 403/500, a main-frame WebView resource error, a rejected navigation and a parsing/read timeout cannot be separated in the feedback attachment. This materially limits the stated import-stall diagnostic purpose. Pass bounded static reason codes through `_failure`, plus numeric HTTP/WebView error codes where applicable, without recording URLs or exception messages.
Closely related coverage gap: `SchoolSession._loaded` still silently swallows the outer JavaScript/evidence exception (`school_session.dart:497-499`). If `runJavaScriptReturningResult` repeatedly throws while the term selection flow is waiting, `finding` remains false and `_scheduleProbe` can exhaust its retries without a `school_evidence`, parse-rejected, or terminal-failure breadcrumb. Record a bounded probe-failed event (exception type only) in that catch so the attached log identifies this stall path.
## Other areas checked
- Checkbox starts unchecked; logs are sent only in the opted-in problem submission.
- Live callers never pass credentials, cookie values, raw URLs/HTML, feedback text or course content. URL classification emits only a fixed page category.
- Cache of `_pendingSubmission` preserves immutable log bytes and the same submission ID across retry/status checking.
- Media validation remains independently limited to three files; backend allows one extra diagnostic part and limits it to 256 KiB.
- Normal generated entries are ASCII and limited to 500 rows with bounded event/field strings, so the exported payload stays below 256 KiB.
- Startup attaches persistence without awaiting it; logger file failures are swallowed and cannot fail app startup.
Non-blocking observation: persistence rewrites the destination directly on each accepted event, so termination during a write can leave a truncated log and lose the previous window. Atomic temporary-file replacement would improve crash resilience, but I have not treated this as a release blocker absent an explicit crash-durability guarantee.
Reference in New Issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Codex