Repository navigation
fix: say why a closed assignment's history is refused - #72
Conversation
Brightspace answers a learner's `mysubmissions` with 403 once an assignment's end date has passed and access is restricted after it, so `student assignment-history` printed only "Permission denied (HTTP 403)." for every closed assignment, as if the session or role were wrong. On that 403 the CLI now reads the folder once. If its end date has passed and EndDateAvailabilityType is 0 (access restricted), it says when the assignment closed, in UTC, and to check released grades with `lighthouse grades COURSE_ID`. Any other availability, an unreadable folder or date, any other status and the instructor `submissions` read keep the original message. The README notes this; its audited .secrets.baseline entry moved three lines.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Droid finished @rabesss's task —— View job The change is well-scoped and correct: on a 403 the fallback reads the folder once, guards every malformed-availability case (wrong types, naive/future/oversized dates, unreadable folder), and re-raises the original error unchanged otherwise. The new error message is built only from a parsed datetime and a validated int course ID, so no server-controlled or secret-bearing text reaches output, and the --json contract is preserved. Tests cover 22 parametrized cases plus the instructor path; no issues found. 0 inline comments posted |
 |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- On a 403 from a learner's
mysubmissionsread,AssessmentAPI.submissions(mine=True) re-reads the dropbox folder once and, when the end date has passed withEndDateAvailabilityType0, raisesAssignmentClosedErrornaming the close time in UTC and pointing atlighthouse grades COURSE_ID. _closed_atfails closed to the original 403 on every ambiguous shape: non-int (incl. bool/string/null) availability type, missing or non-dictAvailability, naive, date-only, future, unparseable, or oversizedEndDate, and any folder-read failure._runsurfacesAssignmentClosedErrorvia its fixed local message instead offormat_user_error; the instructorsubmissionspath (mine=False) is an early direct return, and a test pins that its 403 reads nothing more.- Output stays secret-safe: the message contains only fixed text, a parsed-and-reformatted datetime, and the validated course id — no raw server string is echoed, matching the existing
_safe_http_errordiscipline, and aNEVER_PRINTsentinel assertion pins it. - Docs/baseline: one README sentence;
.secrets.baselinediffs are exactly the line shifts from those edits plusgenerated_at. - Tests: 22-case parametrized matrix covering the closed case (both
Zand offset timestamps) and every fail-closed branch, plus the instructor pin — verified locally (72 passed intests/test_assessment_workflows.py), andscripts/check_secrets.pyruns clean against the regenerated baseline.
One narrow edge noted and accepted: if the session expires between the refused history read and the folder read, _closed_at's except Exception swallows the SessionExpiredError and the user sees the original 403 rather than the session-expiry hint. This requires the session to die between two back-to-back requests while the first still returned a cookie-bearing 403 — not worth the extra branch.
glm-5.3 ｜ 𝕏
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| """When this folder closed, if its end date has passed with access restricted after it.""" | ||
| try: | ||
| folder = self.read("assignment", folder_id) | ||
| except Exception: |
There was a problem hiding this comment.
SUGGESTION: Broad except Exception hides non-transport bugs during the probe
The fallback intent (any unreadable folder keeps the original 403 message) is sound, but this also swallows programming errors in the read path - e.g. a future refactor regression raising TypeError/AttributeError would silently surface as the generic "Permission denied (HTTP 403)." instead of failing loudly in tests. Consider narrowing to the failure types this probe can actually hit, e.g. except (requests.RequestException, NetworkError, SessionExpiredError):.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| availability = folder.get("Availability") if isinstance(folder, dict) else None | ||
| if not isinstance(availability, dict): | ||
| return None | ||
| # AvailabilityDateType 0 restricts access after the end date (1 restricts only submission). |
There was a problem hiding this comment.
SUGGESTION: Comment names AvailabilityDateType, but the field read on the next line is EndDateAvailabilityType
The identifier in this comment matches neither the payload key nor the code, which can mislead the next reader about which value drives the "restricted after end date" behavior.
| # AvailabilityDateType 0 restricts access after the end date (1 restricts only submission). | |
| # EndDateAvailabilityType 0 restricts access after the end date (1 restricts only submission). |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
|
|
||
| class AssignmentClosedError(ValueError): | ||
| """Brightspace refused a learner's own submissions after the folder closed; fixed local message only.""" |
There was a problem hiding this comment.
SUGGESTION: Usable message depends on every caller special-casing this type
Only _run in assessment_commands.py currently maps AssignmentClosedError to str(exc); any future caller that routes it through format_user_error instead degrades to "Permission denied (HTTP 403)." (the status pattern in the message still matches), silently losing the closure explanation and the lighthouse grades hint. Safe today since submissions(mine=True) has a single caller, but worth a note so the coupling to _run is discoverable.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| result = CliRunner().invoke(cli, ["student", "assignment-history", "12", "34", "--json"]) | ||
| assert result.exit_code == 1 | ||
| assert json.loads(result.stdout) == {"course_id": 12, "data": None, | ||
| "error": CLOSED if closed else format_user_error(history)} |
There was a problem hiding this comment.
SUGGESTION: Expected error is self-referential for the 20 non-closed cases
format_user_error(history) appears on both sides of the comparison, so these assertions stay green even if the formatter regresses (e.g. starts leaking transport text or drops the status category). Pinning the literal outputs - like the instructor test below does with "Permission denied (HTTP 403)." - would keep these cases guarding real CLI output.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Merge — suggestions only, none blocking Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (3 files)
Notes
Previous Review Summary (commit 3f8b711)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 3f8b711)Status: 4 Issues Found | Recommendation: Merge — suggestions only, none blocking Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (5 files)
Notes
Reviewed by glm-5.3-flash · Input: 76.1K · Output: 12.9K · Cached: 349K Review guidance: REVIEW.md from base branch |
An aware end date at the edge of the datetime range, such as "0001-01-01T00:00:00+01:00" with access restricted, parsed and passed the checks, then overflowed when converted to UTC, so the CLI printed "Command failed." instead of the original "Permission denied (HTTP 403)." (GPT-6.1 Sol, Grok 4.7). The comparison and the conversion now fall back like any unreadable date; min and max dates are pinned. Tests now pin each fallback's literal message instead of recomputing it with format_user_error (Kilo Code Review), give the closed fixtures a secret-bearing folder name (Muse Spark 1.3), and pin the closed message on stderr with empty stdout without --json (MiMo v2.6 Flash). The code comment names EndDateAvailabilityType (Kilo Code Review). The test file's audited .secrets.baseline entry is back on its original line.
|
Droid encountered an error —— View job |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Re-reviewed the delta since the prior review (3f8b711) — one commit hardening the closed-assignment fallback's datetime handling, plus test strengthening.
- Fixed a real
OverflowErrorescape in_closed_at: the timezone check andastimezone(timezone.utc)conversion now run inside thetrywith the catch widened to(ValueError, OverflowError), so end dates atdatetime's bounds (0001-01-01T00:00:00+01:00,9999-12-31T23:59:00-01:00parse fine but overflow once shifted to UTC) fail closed to the original 403 instead of raising out of the 403 handler into a generic message. - Corrected the availability comment from
AvailabilityDateTypetoEndDateAvailabilityType, matching the field actually read. - Strengthened the test matrix: added
min-date/max-datecases pinning the overflow fix, switched every case to asserting the exact expected error string (404 and network messages included, no longer derived fromformat_user_error), embedded aNEVER_PRINTsentinel in the folderNameto pin that it never reaches output, and added a test pinning the closed message to stderr with empty stdout on the non---jsonpath — matchingfail()'serr=Truecontract. - Reverted the test-only
format_user_errorimport, restoring the import block (and the.secrets.baselinetest-file entry) to the base shape.
Verified locally: tests/test_assessment_workflows.py 75 passed, ruff check clean on the changed files, scripts/check_secrets.py clean against the regenerated baseline.
glm-5.3 ｜ 𝕏
1 participant
What
Brightspace refuses a learner's own submission history (
mysubmissions, HTTP 403) once an assignment's end date has passed and its availability restricts access after that date.student assignment-historyprinted only "Permission denied (HTTP 403)." for every such assignment, which reads like a broken session or a wrong role.On that 403, the CLI now reads the assignment folder once. If its end date has passed and
EndDateAvailabilityTypeis 0 (access restricted after the end date), the error says when the assignment closed (in UTC) and to check released grades withlighthouse grades COURSE_ID:Everything else keeps the original message: type 1 (only submission restricted), 2 (hidden), a missing or non-integer type, a future, naive, date-only, oversized or out-of-range end date, an unreadable folder, any other status, and the instructor
submissionsread (which reads nothing more).Docs
.secrets.baseline: the audited README entry moved from line 340 to 343. Nothing else changed.Testing
Zand a+05:30offset normalised to UTC), every non-closed availability above (including year 0001 and 9999 dates that overflow in UTC), a non-dict folder or availability, a folder read that fails with 403 or a network error, anHTTPErrorwith no response, 404, andNetworkError. It checks the exact JSON error, which reads were made, and that no secret sentinel (in the folder name, the date or an exception) reaches the output. Two more tests pin the message on stderr without--jsonand the instructorsubmissions403. The new tests fail without the fix.gradeslists the course's items.