github-starred/cs249r_book
2
0
Fork 0
mirror of https://github.com/harvard-edge/cs249r_book.git synced 2026-07-15 21:28:33 -05:00
Code Issues 276 Packages Projects Releases 26 Wiki Activity

[PR #1950] [MERGED] fix(tinytorch): nbgrader generate can silently overwrite a student's notebook #34674

New Issue
Closed
opened 2026-07-14 19:28:37 -05:00 by GiteaMirror · 0 comments
GiteaMirror commented 2026-07-14 19:28:37 -05:00
Owner
Copy Link

📋 Pull Request Information

Original PR: https://github.com/harvard-edge/cs249r_book/pull/1950
Author: @Shashank-Tripathi-07
Created: 7/7/2026
Status: ✅ Merged
Merged: 7/10/2026
Merged by: @profvjreddi

Base: dev ← Head: fix/nbgrader-notebook-overwrite

📝 Commits (1)

  • 2d0e5ac fix(tinytorch): nbgrader generate can silently overwrite a student's notebook

📊 Changes

2 files changed (+33 additions, -11 deletions)

View changed files

📝 tinytorch/.gitignore (+1 -0)
📝 tinytorch/tito/commands/nbgrader.py (+32 -11)

📄 Description

Summary

  • _ensure_module_notebook() regenerates modules/<name>/<name>.ipynb from src/<name>.py whenever the source file's mtime is newer than the notebook's -- but that exact path is the same notebook tito module start/resume open for students to edit (confirmed against workflow.py lines 518, 777, 1190, 1239, which all construct the identical modules/<module>/<short_name>.ipynb path).
  • A routine git pull that bumps a source file's mtime, followed by an instructor running tito nbgrader generate --all (or per-module) on a shared checkout, silently overwrote any student notebook whose file mtime was older than the source -- no backup, no confirmation, just a benign "Source file is newer; converting..." message.

Fix

Never write to the student-facing notebook path once it exists. If it needs regenerating from a newer source, stage the fresh conversion into a private .nbgrader_staging/ cache instead, and read from there for nbgrader's own assignment staging (_generate_single_module already just reads whatever path _ensure_module_notebook returns, so no other caller needed to change). First-time creation (no notebook yet) and the already-up-to-date case are unaffected -- both still return the direct modules/ path with no staging detour.

Test plan

  • Verified against the real _ensure_module_notebook (jupytext subprocess call mocked, everything else real file I/O) across three scenarios:
    1. Notebook missing entirely -> created directly at the notebook path, same as before.
    2. Notebook exists and is already up to date -> zero conversion calls, notebook untouched (asserted no jupytext invocation happens).
    3. Notebook exists but source is newer -> student notebook content is provably left byte-for-byte untouched; a fresh conversion is written to .nbgrader_staging/<module>/<name>.ipynb instead.
  • Added .nbgrader_staging/ to .gitignore since it's a generated cache.
  • pre-commit run passes (only the known, unrelated, always-run book-check-cli-contract hook fails locally due to a Windows interpreter mismatch in this dev environment).

🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.

## 📋 Pull Request Information **Original PR:** https://github.com/harvard-edge/cs249r_book/pull/1950 **Author:** [@Shashank-Tripathi-07](https://github.com/Shashank-Tripathi-07) **Created:** 7/7/2026 **Status:** ✅ Merged **Merged:** 7/10/2026 **Merged by:** [@profvjreddi](https://github.com/profvjreddi) **Base:** `dev` ← **Head:** `fix/nbgrader-notebook-overwrite` --- ### 📝 Commits (1) - [`2d0e5ac`](https://github.com/harvard-edge/cs249r_book/commit/2d0e5ac67c35d03237beee895e916914f4080762) fix(tinytorch): nbgrader generate can silently overwrite a student's notebook ### 📊 Changes **2 files changed** (+33 additions, -11 deletions) <details> <summary>View changed files</summary> 📝 `tinytorch/.gitignore` (+1 -0) 📝 `tinytorch/tito/commands/nbgrader.py` (+32 -11) </details> ### 📄 Description ## Summary - `_ensure_module_notebook()` regenerates `modules/<name>/<name>.ipynb` from `src/<name>.py` whenever the source file's mtime is newer than the notebook's -- but that exact path is the same notebook `tito module start`/`resume` open for students to edit (confirmed against `workflow.py` lines 518, 777, 1190, 1239, which all construct the identical `modules/<module>/<short_name>.ipynb` path). - A routine `git pull` that bumps a source file's mtime, followed by an instructor running `tito nbgrader generate --all` (or per-module) on a shared checkout, silently overwrote any student notebook whose file mtime was older than the source -- no backup, no confirmation, just a benign "Source file is newer; converting..." message. ## Fix Never write to the student-facing notebook path once it exists. If it needs regenerating from a newer source, stage the fresh conversion into a private `.nbgrader_staging/` cache instead, and read from there for nbgrader's own assignment staging (`_generate_single_module` already just reads whatever path `_ensure_module_notebook` returns, so no other caller needed to change). First-time creation (no notebook yet) and the already-up-to-date case are unaffected -- both still return the direct `modules/` path with no staging detour. ## Test plan - [x] Verified against the real `_ensure_module_notebook` (jupytext subprocess call mocked, everything else real file I/O) across three scenarios: 1. Notebook missing entirely -> created directly at the notebook path, same as before. 2. Notebook exists and is already up to date -> zero conversion calls, notebook untouched (asserted no jupytext invocation happens). 3. Notebook exists but source is newer -> student notebook content is provably left byte-for-byte untouched; a fresh conversion is written to `.nbgrader_staging/<module>/<name>.ipynb` instead. - [x] Added `.nbgrader_staging/` to `.gitignore` since it's a generated cache. - [x] `pre-commit run` passes (only the known, unrelated, always-run `book-check-cli-contract` hook fails locally due to a Windows interpreter mismatch in this dev environment). --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>
GiteaMirror added the pull-request label 2026-07-14 19:28:37 -05:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
area: book
area: collabs
area: kits
area: labs
area: socratiq
area: staffml
area: tinytorch
area: tools
area: website
bug
dependencies
format: epub
format: pdf
javascript
link-health
link-rot
priority-high
pull-request
Mirrored from GitHub Pull Request
 staffml
type: bug
type: citation
type: code
type: errata
type: improvement
type: new
type: question
vault-sli
No Label pull-request
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
GiteaMirror ninjasurge
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: github-starred/cs249r_book#34674
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?