Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,15 @@ Outputs are consistently routed to a `[theme]_series/` directory in both **PNG**

## 6. Git Workflow Rules

**Never commit directly to `main`.** All changes must go through a pull request, regardless of size or urgency. This applies to agents, automated fixes, and humans alike.

The correct workflow for any change:
1. Create a branch from `origin/main`: `git checkout -b <branch-name> origin/main`
2. Make commits on that branch
3. Push the branch: `git push -u origin <branch-name>`
4. Open a PR: `gh pr create ...`
5. Do **not** push to `main` directly

**Before pushing any commit, always verify the current branch has not been merged:**
```bash
git fetch origin
Expand All @@ -71,3 +80,16 @@ If your current branch tip appears in `origin/main`, it has been merged. **Do no
3. Push the new branch and open a fresh PR

**Never accumulate multiple unrelated fixes on one branch.** Each PR should be focused on a single concern so it can be merged independently without blocking other work.

---

## 7. Automated Analysis & Fix Workflow

When an agent performs a codebase audit and creates GitHub issues, the following pattern must be followed:

1. **Analysis**: Spawn a read-only agent to identify bugs/deficiencies. Each finding must include a reproduction scenario and justification before an issue is filed.
2. **Issue creation**: File GitHub issues with the `Claude-identified` label. One issue per distinct bug.
3. **Fix branch**: Create a single branch (`fix/claude-identified-bugs` or similar) from `origin/main` **before** making any changes.
4. **Fix & commit**: Apply fixes on the branch and commit. Do not touch `main`.
5. **PR**: Push the branch and open a PR referencing the issues (e.g., `Closes #77, #78`). Do not close issues manually — let the PR merge close them via GitHub keywords.
6. **No direct-to-main commits**: Even if the fix seems trivial, it must go through a PR so the owner can review before merging.
4 changes: 2 additions & 2 deletions src/agent.py
Original file line number Diff line number Diff line change
Expand Up @@ -724,7 +724,7 @@ def generate_weekly_plan(student_id: str, grade_level: int, subject: str) -> dic
raise ValueError(f"Student with id '{student_id}' not found")

# Parse plan_rules_blob to get rules
rules = json.loads(student_profile["plan_rules_blob"])
rules = json.loads(student_profile["plan_rules_blob"] or "{}")

# Get standards for the student. We request more than 5 to have flexibility
# in how they're distributed across the week. Some complex standards may need
Expand Down Expand Up @@ -763,7 +763,7 @@ def generate_weekly_plan(student_id: str, grade_level: int, subject: str) -> dic

generation_logger = GenerationLogger(student_id, grade_level, subject)

today = datetime.now()
today = datetime.now(UTC)
monday = today - timedelta(days=today.weekday())
week_of = monday.strftime("%Y-%m-%d")
plan_id = f"plan_{student_id}_{week_of}"
Expand Down
8 changes: 8 additions & 0 deletions src/db_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -252,9 +252,17 @@ def delete_student(student_id: str) -> bool:
True if the student was deleted, False if not found
"""
conn = sqlite3.connect(DB_FILE)
conn.execute("PRAGMA foreign_keys = ON")

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[CRITICAL — regression] PRAGMA foreign_keys = ON activates the FK constraint from ingest_standards.py's packet_feedback schema:

FOREIGN KEY (student_id) REFERENCES student_profiles(student_id)

This has no ON DELETE CASCADE and the weekly_packets delete above does not cascade to packet_feedback (because in the actual live DB, packet_feedback has an FK to student_profiles, not to weekly_packets). So the sequence is:

  1. DELETE FROM weekly_packets WHERE student_id = ? — orphans packet_feedback rows (no cascade)
  2. DELETE FROM student_profiles WHERE student_id = ?raises sqlite3.IntegrityError: FOREIGN KEY constraint failed because packet_feedback rows still reference this student_id

Any student who has ever submitted feedback is now undeletable after this PR. The endpoint returns 500 instead of 204.

Suggestion: rather than enabling PRAGMA here, explicitly delete packet_feedback rows before deleting student_profiles (similar to how weekly_packets is already handled), or remove the conflicting FK from ingest_standards.py and rely solely on packet_store.py's schema.

cursor = conn.cursor()

try:
# Delete packets first — their FK cascade removes daily_lessons,
# worksheet_artifacts, and packet_feedback automatically.
# weekly_packets may not exist in minimal test DBs, so ignore if absent.
try:
cursor.execute("DELETE FROM weekly_packets WHERE student_id = ?", (student_id,))
except sqlite3.OperationalError:

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM] This except sqlite3.OperationalError: pass is too broad. It was intended to handle "no such table: weekly_packets" in minimal test DBs, but it will also silently swallow:

  • A disk-full error
  • A locked database
  • A corrupted DB page
  • A misspelled table name after a future rename

If the goal is to handle missing tables in tests, consider either:

except sqlite3.OperationalError as e:
    if "no such table" not in str(e):
        raise

or better, fix the test fixtures to always call ensure_schema() before delete_student.

pass
cursor.execute(
"DELETE FROM student_profiles WHERE student_id = ?",
(student_id,),
Expand Down
13 changes: 8 additions & 5 deletions src/feedback_processor.py
Original file line number Diff line number Diff line change
Expand Up @@ -168,8 +168,10 @@ def process_quantity_feedback(
elif quantity_feedback == 2:
current_bias += 0.3

# Clamp to [-1.0, 1.0]
prefs["activity_bias"] = max(-ACTIVITY_BIAS_CLAMP, min(current_bias, ACTIVITY_BIAS_CLAMP))
# Clamp to [-1.0, 1.0] and round to avoid floating-point drift
prefs["activity_bias"] = round(
max(-ACTIVITY_BIAS_CLAMP, min(current_bias, ACTIVITY_BIAS_CLAMP)), 6
)

return json.dumps(plan_rules)

Expand Down Expand Up @@ -200,14 +202,15 @@ def reverse_quantity_feedback(plan_rules_blob: str, old_quantity_feedback: int)
elif old_quantity_feedback == -1:
current_bias += 0.15
elif old_quantity_feedback == 0:
# Reverse the *0.9 decay by dividing; safe because bias could be 0
current_bias = current_bias / 0.9 if current_bias != 0 else 0.0
current_bias = round(current_bias / 0.9, 6) if current_bias != 0 else 0.0
elif old_quantity_feedback == 1:
current_bias -= 0.15
elif old_quantity_feedback == 2:
current_bias -= 0.3

prefs["activity_bias"] = max(-ACTIVITY_BIAS_CLAMP, min(current_bias, ACTIVITY_BIAS_CLAMP))
prefs["activity_bias"] = round(
max(-ACTIVITY_BIAS_CLAMP, min(current_bias, ACTIVITY_BIAS_CLAMP)), 6
)
return json.dumps(plan_rules)


Expand Down
4 changes: 2 additions & 2 deletions src/logic.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,8 @@ def get_filtered_standards(
raise ValueError(f"Student with id '{student_id}' not found")

# Step c: Parse the JSON blobs
progress_blob = json.loads(student_profile["progress_blob"])
plan_rules_blob = json.loads(student_profile["plan_rules_blob"])
progress_blob = json.loads(student_profile["progress_blob"] or "{}")
plan_rules_blob = json.loads(student_profile["plan_rules_blob"] or "{}")

# Step d: Extract mastered standards
mastered_standards = progress_blob.get("mastered_standards", [])
Expand Down
2 changes: 1 addition & 1 deletion src/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -748,7 +748,7 @@ def get_student_progress_map(student_id: str, subject: str, prune: bool = True):
if not profile:
raise HTTPException(status_code=404, detail="Student not found")

progress = json.loads(profile["progress_blob"])
progress = json.loads(profile["progress_blob"] or "{}")
mastered = progress.get("mastered_standards", [])

graph = load_from_db(str(PROJECT_ROOT / "curriculum.db"), subject)
Expand Down
7 changes: 1 addition & 6 deletions src/packet_store.py
Original file line number Diff line number Diff line change
Expand Up @@ -160,10 +160,6 @@ def _build_summary(weekly_plan: Mapping[str, Any]) -> dict[str, Any]:
}


def _delete_existing_packet(conn: sqlite3.Connection, packet_id: str) -> None:
conn.execute("DELETE FROM weekly_packets WHERE packet_id = ?", (packet_id,))


def _insert_weekly_packet(
conn: sqlite3.Connection,
weekly_plan: Mapping[str, Any],
Expand All @@ -176,7 +172,7 @@ def _insert_weekly_packet(

conn.execute(
"""
INSERT INTO weekly_packets (
INSERT OR REPLACE INTO weekly_packets (
packet_id,
student_id,
grade_level,
Expand Down Expand Up @@ -305,7 +301,6 @@ def save_weekly_packet(
raise ValueError("weekly_plan must include plan_id")

with _get_connection() as conn:
_delete_existing_packet(conn, packet_id)
_insert_weekly_packet(conn, weekly_plan, status)
_persist_daily_lessons(conn, packet_id, weekly_plan.get("daily_plan", []))

Expand Down
13 changes: 9 additions & 4 deletions src/trio_generator.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,17 @@
logger = logging.getLogger(__name__)


def _get_grade_level(student_id: str) -> int:
"""Return grade_level from the student's most recent weekly packet, or 0."""
def _get_grade_level(student_id: str, metadata_fallback: dict | None = None) -> int:
"""Return grade_level from the student's most recent weekly packet.

Falls back to metadata_fallback["grade_level"] if no packets exist, then to 1.
"""
packets, _ = list_weekly_packets(student_id, limit=1, offset=0)
if packets:
return packets[0].get("grade_level", 0)
return 0
if metadata_fallback:
return int(metadata_fallback.get("grade_level", 1))
return 1

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW] The fallback sentinel changed from 0 to 1. While better than returning 0 blindly, grade level 0 is Kindergarten per the standards schema. A new Kindergarten student whose metadata_blob has no grade_level key, or whose profile has no metadata at all, will get grade-1 lesson plans generated instead of grade-0 ones.

The metadata_fallback path added above handles the common case correctly. This final fallback (return 1) is fine if grade 0 students are always guaranteed to have grade_level in their metadata blob — but that guarantee isn't enforced anywhere.



def generate_trio_for_student(student_id: str) -> None:
Expand All @@ -30,7 +35,7 @@ def generate_trio_for_student(student_id: str) -> None:
name = metadata.get("name", student_id)

try:
grade_level = _get_grade_level(student_id)
grade_level = _get_grade_level(student_id, metadata_fallback=metadata)
subjects = pick_subjects(student_id)

with ThreadPoolExecutor(max_workers=3) as executor:
Expand Down
9 changes: 7 additions & 2 deletions tests/test_trio_generator.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,11 @@ def test_get_grade_level_from_most_recent_packet():
assert trio_generator._get_grade_level("s1") == 3


def test_get_grade_level_defaults_to_zero_when_no_packets():
def test_get_grade_level_defaults_to_one_when_no_packets():
with patch("trio_generator.list_weekly_packets", return_value=([], False)):
assert trio_generator._get_grade_level("s1") == 0
assert trio_generator._get_grade_level("s1") == 1


def test_get_grade_level_uses_metadata_fallback_when_no_packets():
with patch("trio_generator.list_weekly_packets", return_value=([], False)):
assert trio_generator._get_grade_level("s1", metadata_fallback={"grade_level": 4}) == 4
Loading