diff --git a/.secrets.baseline b/.secrets.baseline index b5408ea22..f5ee2dcea 100644 --- a/.secrets.baseline +++ b/.secrets.baseline @@ -137,7 +137,7 @@ { "type": "Hex High Entropy String", "filename": "agents/manifest.json", - "hashed_secret": "29931b5dfbdbd174970fe96043c4a2faa0f66211", + "hashed_secret": "5617d8fd88638c00407226b41d3342ad2225a25c", "is_verified": false, "line_number": 5 }, @@ -151,7 +151,7 @@ { "type": "Hex High Entropy String", "filename": "agents/manifest.json", - "hashed_secret": "9c4a8143009ea914986ea08c9f68bb5c6d6a691e", + "hashed_secret": "55cf0fc92116d53cb3d392173404b48bcf688db6", "is_verified": false, "line_number": 13 }, @@ -172,14 +172,14 @@ { "type": "Hex High Entropy String", "filename": "agents/manifest.json", - "hashed_secret": "110a0b5b9eb28137140669d5b4fdaf083a70a30f", + "hashed_secret": "57dfc788ed2e518411553353dc67065fcf7961ed", "is_verified": false, "line_number": 29 }, { "type": "Hex High Entropy String", "filename": "agents/manifest.json", - "hashed_secret": "868f2d2b7204c9567e125166863cd7fe2a24582f", + "hashed_secret": "7137f7bb0acc961407878db7c3545234a17a6d19", "is_verified": false, "line_number": 37 }, @@ -207,37 +207,44 @@ { "type": "Hex High Entropy String", "filename": "agents/manifest.json", - "hashed_secret": "4e0abd652b07617b2ac184d46f040a7cabdf4a27", + "hashed_secret": "55b5135330fd4896f3cc433497080730bcec8403", "is_verified": false, - "line_number": 65 + "line_number": 61 }, { "type": "Hex High Entropy String", "filename": "agents/manifest.json", - "hashed_secret": "b37f8d4aca0f85435b52e167ee9f09280a3f9593", + "hashed_secret": "4e0abd652b07617b2ac184d46f040a7cabdf4a27", "is_verified": false, "line_number": 69 }, { "type": "Hex High Entropy String", "filename": "agents/manifest.json", - "hashed_secret": "e0a1b3d7e38a074c21ec31895cdfbe5aa1019e22", + "hashed_secret": "b37f8d4aca0f85435b52e167ee9f09280a3f9593", "is_verified": false, "line_number": 73 }, { "type": "Hex High Entropy String", "filename": "agents/manifest.json", - "hashed_secret": "c5500010d104bbe79247185dc8f18325d26bce2b", + "hashed_secret": "68c9c672f0270cc052ebe5fbd253d7fdb58b5522", "is_verified": false, "line_number": 77 }, + { + "type": "Hex High Entropy String", + "filename": "agents/manifest.json", + "hashed_secret": "c5500010d104bbe79247185dc8f18325d26bce2b", + "is_verified": false, + "line_number": 81 + }, { "type": "Hex High Entropy String", "filename": "agents/manifest.json", "hashed_secret": "ee12a35a3779f33af762a202e2f8dfa4781ae562", "is_verified": false, - "line_number": 85 + "line_number": 89 } ], "agents/mcp/README.md": [ @@ -2049,5 +2056,5 @@ } ] }, - "generated_at": "2026-09-23T07:00:46Z" + "generated_at": "2026-09-23T10:41:10Z" } diff --git a/CHANGELOG.md b/CHANGELOG.md index bb5e47261..15d8e152a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,20 @@ experience may change before `1.0.0`. separately through the skills CLI, not by `studyloop install agents`; its guide is published on the docs site as *Lesson Study Notes* (`docs/study-notes-skill.md`). +- The mentor writes to the learning tier (issue #38, item 1 of the + learning-tier plan). A `record_teachback` MCP tool records a teach-back + score through the same validator the `studyloop teachback` CLI uses (the + CLI now delegates to it; its messages are unchanged), so a mentor session's + teach-back reaches tomorrow's recommendation instead of stopping at the + conversation. One `agents/shared/recording-protocol.md` carries a + machine-readable trigger table naming exactly the four append-only writers + (`log_topic`, `log_struggle`, `record_teachback`, `record_plan_learning`); + every harness definition and the built live persona name the same four, + and parity tests pin the names, the protocol hash and the absence of any + new pre-approval — recording stays prompt-per-call on every harness, by + owner decision. Scheduling mutators never fire from the protocol. A + child-process isolation guard proves the four writers land nowhere outside + the sandbox database. Owner decisions 1–5 are recorded in the plan (#34). ### Changed @@ -62,6 +76,13 @@ experience may change before `1.0.0`. no learner saw the split; `spaced_repetition_due()` gains a keyword-only `now` for callers that already hold an instant, defaulting to the wall clock for everyone else. +- A browser tab rejoining a session whose transport had already drained no + longer leaves *Task exception was never retrieved … StopAsyncIteration* in + the server log. The WebSocket pump pulls each transport event as its own + future; when a newer socket took the slot, or the client closed while the + drained future was already complete, the pump exited without reading that + future's `StopAsyncIteration`. The pump now retrieves the result of a done + pull future on every exit path. ## [0.5.0] - 2026-09-21 diff --git a/agents/claude/socratic-mentor.md b/agents/claude/socratic-mentor.md index d635c8ba5..14ef15dae 100644 --- a/agents/claude/socratic-mentor.md +++ b/agents/claude/socratic-mentor.md @@ -2,7 +2,7 @@ name: socratic-mentor description: AuDHD-aware Socratic study mentor with spaced repetition, energy-adaptive sessions, network→DE concept bridges, and Clean Code/GoF discovery patterns category: communication -tools: Read, Write, Grep, Bash +tools: Read, Write, Grep, Bash, mcp__studyloop__get_concept_context, mcp__studyloop__get_study_history, mcp__studyloop__get_next_action, mcp__studyloop__get_topic_suggestions, mcp__studyloop__get_active_topics, mcp__studyloop__log_topic, mcp__studyloop__log_struggle, mcp__studyloop__record_teachback, mcp__studyloop__record_plan_learning --- # StudyLoop @@ -19,6 +19,7 @@ See `agents/shared/knowledge-bridging.md` for configurable domain bridges. See `agents/shared/break-science.md` for active break protocol. See `agents/shared/wind-down-protocol.md` for end-of-session consolidation. See `agents/shared/teach-back-protocol.md` for teach-back scoring. +See `agents/shared/recording-protocol.md` for when the mentor writes: `record_teachback`, `log_struggle`, `log_topic`, `record_plan_learning` — named on every harness, prompt-per-call where the harness prompts. ## Identity diff --git a/agents/codex/AGENTS.md b/agents/codex/AGENTS.md index a9551534b..7ee213f73 100644 --- a/agents/codex/AGENTS.md +++ b/agents/codex/AGENTS.md @@ -12,6 +12,7 @@ See `agents/shared/knowledge-bridging.md` for configurable domain bridges. See `agents/shared/break-science.md` for active break protocol. See `agents/shared/wind-down-protocol.md` for end-of-session consolidation. See `agents/shared/teach-back-protocol.md` for teach-back scoring. +See `agents/shared/recording-protocol.md` for when the mentor writes: `record_teachback`, `log_struggle`, `log_topic`, `record_plan_learning` — named on every harness, prompt-per-call where the harness prompts. ## Identity diff --git a/agents/kiro/study-mentor/persona.md b/agents/kiro/study-mentor/persona.md index 0a4a249f3..f047b400c 100644 --- a/agents/kiro/study-mentor/persona.md +++ b/agents/kiro/study-mentor/persona.md @@ -29,7 +29,7 @@ Follow the unified session protocol in `agents/shared/session-protocol.md`: - One question at a time. Stop. Wait for response. - Network→DE bridges for every new concept - Max 3-4 concepts per explanation, TL;DR at top, mermaid diagrams for structure -- Record progress: `uv run tutor-checkpoint --notes ""` +- Record what the learner agreed, when `agents/shared/recording-protocol.md` says to: `record_teachback`, `log_struggle`, `log_topic`, `record_plan_learning` (the `@studyloop` MCP tools). Say so in one line, then the next question. ## Session Types @@ -76,10 +76,12 @@ studyloop review # What's due for review? studyloop struggles # Recurring struggle topics studyloop wins # Show learning wins -# Progress tracking +# Progress tracking — the mentor writes through MCP (see recording-protocol.md): +# @studyloop/record_teachback, @studyloop/log_struggle, @studyloop/log_topic, +# @studyloop/record_plan_learning. The CLI below is the human's path to the same rows. studyloop progress "" -t -c -uv run tutor-checkpoint --notes "" -session-query search "" # read back prior checkpoints for a skill +studyloop teachback "" -t --score "3,3,4,3,2" --type structured +session-query search "" # read back prior sessions for a skill # Cross-machine sync studyloop state pull # Get latest from hub @@ -100,3 +102,4 @@ Configured in `~/.config/studyloop/config.yaml` - `agents/shared/audhd-framework.md` — Complete AuDHD cognitive support - `agents/shared/socratic-engine.md` — Questioning methodology - `agents/shared/network-bridges.md` — Network→DE concept bridges +- `agents/shared/recording-protocol.md` — When the mentor writes (`record_teachback`, `log_struggle`, `log_topic`, `record_plan_learning`) diff --git a/agents/manifest.json b/agents/manifest.json index 23253c4e3..d3a16dbe9 100644 --- a/agents/manifest.json +++ b/agents/manifest.json @@ -2,92 +2,96 @@ "version": 1, "agents": { "claude/socratic-mentor.md": { - "hash": "b0be77358df99a1b", - "updated": "2026-09-14" + "hash": "2bd4f5ebfdd9f045", + "updated": "2026-09-23" }, "claude/study-plan-architect.md": { "hash": "19f1a1c0853cacf9", - "updated": "2026-09-18" + "updated": "2026-09-23" }, "codex/AGENTS.md": { - "hash": "7e6c1a0d534b65f7", - "updated": "2026-09-14" + "hash": "4a9d027b2cf0207e", + "updated": "2026-09-23" }, "kiro/study-mentor.json": { "hash": "21f51c19993e7ce4", - "updated": "2026-09-16" + "updated": "2026-09-23" }, "kiro/study-plan-architect.json": { "hash": "ab42104634985d3e", - "updated": "2026-09-16" + "updated": "2026-09-23" }, "opencode/plugins/studyloop-session-export.js": { "hash": "769ed9efdda111a9", - "updated": "2026-09-14" + "updated": "2026-09-23" }, "opencode/study-mentor.md": { - "hash": "f91a0b52fd59e80f", - "updated": "2026-09-14" + "hash": "b7acf2cea171b4f1", + "updated": "2026-09-23" }, "opencode/study-plan-architect.md": { "hash": "40057646dc674008", - "updated": "2026-09-18" + "updated": "2026-09-23" }, "pi/AGENTS.md": { - "hash": "03355b0aa919ef6b", - "updated": "2026-09-14" + "hash": "f8528bdcc97b7909", + "updated": "2026-09-23" }, "pi/extensions/studyloop-session-export.ts": { "hash": "5169fade780f0229", - "updated": "2026-09-14" + "updated": "2026-09-23" }, "shared/audhd-framework.md": { "hash": "8b694064b100741f", - "updated": "2026-09-14" + "updated": "2026-09-23" }, "shared/break-science.md": { "hash": "74541a44431f7f6a", - "updated": "2026-09-14" + "updated": "2026-09-23" }, "shared/knowledge-bridging.md": { "hash": "33edec1cceac2e36", - "updated": "2026-09-14" + "updated": "2026-09-23" }, "shared/network-bridges.md": { "hash": "8af4732b77cc15ae", - "updated": "2026-09-14" + "updated": "2026-09-23" + }, + "shared/recording-protocol.md": { + "hash": "21d8b0f8d20e0737", + "updated": "2026-09-23" }, "shared/session-db-mandate.md": { "hash": "bdf22a3da57da972", - "updated": "2026-09-14" + "updated": "2026-09-23" }, "shared/session-protocol.md": { "hash": "7f4f0c2cf9e3eb53", - "updated": "2026-09-14" + "updated": "2026-09-23" }, "shared/socratic-engine.md": { "hash": "42738fba9479ec14", - "updated": "2026-09-14" + "updated": "2026-09-23" }, "shared/teach-back-protocol.md": { - "hash": "09ee534106322244", - "updated": "2026-09-20" + "hash": "5512b099237e8f30", + "updated": "2026-09-23" }, "shared/wind-down-protocol.md": { "hash": "5b1ec3303b8d1086", - "updated": "2026-09-14" + "updated": "2026-09-23" }, "skills/studyloop-session-memory/SKILL.md": { "hash": "8d278dd01b1aa82d", - "updated": "2026-09-14" + "updated": "2026-09-23" }, "skills/studyloop-xtiles-wind-down/SKILL.md": { "hash": "a2e84febb865f24d", - "updated": "2026-09-14" + "updated": "2026-09-23" }, "skills/studyloop-xtiles-wind-down/references/harnesses.md": { "hash": "87a8ab3c4a133afa", - "updated": "2026-09-14" + "updated": "2026-09-23" } } } diff --git a/agents/mcp/README.md b/agents/mcp/README.md index 1204aa093..d1fdb5dea 100644 --- a/agents/mcp/README.md +++ b/agents/mcp/README.md @@ -153,7 +153,7 @@ Requires a Google Cloud project with Calendar API enabled. See [setup guide](htt ## studyloop-mcp (Study tools) -The `studyloop-mcp` server exposes 32 MCP tools: courses and review cards, the study backlog and +The `studyloop-mcp` server exposes 33 MCP tools: courses and review cards, the study backlog and progress signals, lesson browsing, the live session, the `now` recommendation, and the learner's study plans (nine lifecycle tools plus `record_plan_learning`, every one through the same plan application layer the CLI and Web UI use — see `docs/agent-install.md`, "Study-plan tools over @@ -199,6 +199,7 @@ server NAME is `studyloop`; `studyloop-mcp` is the console-script COMMAND, never | `get_active_topics` | The AuDHD three-topic active set vs the remaining backlog | | `log_topic` | Record a learning / struggling / insight signal mid-session | | `log_struggle` | Record a topic the learner struggled with, for later study | +| `record_teachback` | Record the five agreed teach-back scores for a concept — the mentor's write after a teach-back | | `get_concept_context` | Concept dependency edges for a topic, with per-edge provenance and `coverage` — the prerequisite structure a mentor sequences from | | `get_next_action` | The same "what now?" recommendation the web `/api/now` endpoint gives — plan-aware when a plan is active | | `get_lesson_tree` | Browse the course-material tree: providers → courses → lessons | diff --git a/agents/opencode/study-mentor.md b/agents/opencode/study-mentor.md index da48f7e2e..c1c865b64 100644 --- a/agents/opencode/study-mentor.md +++ b/agents/opencode/study-mentor.md @@ -30,6 +30,7 @@ See `agents/shared/knowledge-bridging.md` for configurable domain bridges. See `agents/shared/break-science.md` for active break protocol. See `agents/shared/wind-down-protocol.md` for end-of-session consolidation. See `agents/shared/teach-back-protocol.md` for teach-back scoring. +See `agents/shared/recording-protocol.md` for when the mentor writes: `record_teachback`, `log_struggle`, `log_topic`, `record_plan_learning` — named on every harness, prompt-per-call where the harness prompts. ## Identity diff --git a/agents/pi/AGENTS.md b/agents/pi/AGENTS.md index bf0f5bb88..9558858ac 100644 --- a/agents/pi/AGENTS.md +++ b/agents/pi/AGENTS.md @@ -12,6 +12,7 @@ See `~/.agents/shared/knowledge-bridging.md` for configurable domain bridges. See `~/.agents/shared/break-science.md` for active break protocol. See `~/.agents/shared/wind-down-protocol.md` for end-of-session consolidation. See `~/.agents/shared/teach-back-protocol.md` for teach-back scoring. +See `~/.agents/shared/recording-protocol.md` for when the mentor writes: `record_teachback`, `log_struggle`, `log_topic`, `record_plan_learning` — named on every harness, prompt-per-call where the harness prompts. ## Session Memory diff --git a/agents/shared/personas/co-study.md b/agents/shared/personas/co-study.md index 9a3005c25..33451bd47 100644 --- a/agents/shared/personas/co-study.md +++ b/agents/shared/personas/co-study.md @@ -31,6 +31,12 @@ Always include a studyloop command in these situations — not as a separate blo Fill in the note with something specific to what they actually said — never leave it as a generic placeholder. +The same four triggers, when the student confirms rather than runs the command themselves, are yours to write +through the `studyloop-mcp` tools — `log_topic` at wind-down, `log_struggle` after two stuck rounds, +`record_teachback` only for scores the student agreed, `record_plan_learning` only for a plan they are working +against. `agents/shared/recording-protocol.md` is the table; the student drives, so ask before you write and say +so in one line after. + ## Energy Adaptation Check session energy level and elapsed time before responding: diff --git a/agents/shared/personas/study.md b/agents/shared/personas/study.md index e416f6f0c..ee13e8815 100644 --- a/agents/shared/personas/study.md +++ b/agents/shared/personas/study.md @@ -35,6 +35,23 @@ Status values: `learning` (in progress), `win` (understood), `insight` (aha mome **When logging, show the command inline.** Example: "That's a win — run: `studyloop topic "Decorators" --status win --note "grasped wrapping pattern"`" +## Recording — what you write yourself + +`agents/shared/recording-protocol.md` is the one table that says when *you* write to the +learning tier, through the `studyloop-mcp` tools (they may prompt the learner to approve the call; +that is the harness's setting, not yours to change). Four writers, four triggers: + +- a teach-back has ended, you proposed five rubric scores, and the learner **agreed** them → + `record_teachback` (concept, topic, the five scores in order, the review type). Not agreed, not recorded. +- two rounds stuck on one point → `log_struggle` (the question) +- wind-down, one call per concept touched, with the status the learner confirms → `log_topic` +- the session ran against an active plan and the learner agrees one line for its record → `record_plan_learning` + +A turn that matches none of these writes nothing. After a write, one short line — *"Recorded: window frame, +structured, 15/20."* — then the next question. If the tool refuses or the learner declines the prompt, say so in +one line and continue; never retry silently. The `studyloop topic` commands above remain the learner's own, +visible path to the same rows. + ## Win Recognition — IMPORTANT When a student demonstrates correct understanding, makes a connection, or has an aha moment: @@ -83,6 +100,10 @@ studyloop content generate-cards ~/Obsidian/Personal/Study/ --c Use local study sources, generated review artefacts, and session history by default. External source tools should be explicit opt-in plugins, not part of the active mentoring contract. **Progress & Review:** +- `record_teachback` — record the five agreed teach-back scores for a concept (see Recording above) +- `log_topic` — record a learning / struggling / insight / win / parked signal for a concept +- `log_struggle` — record a question the learner was stuck on, for later study +- `record_plan_learning` — append one learning record to an active study plan - `record_study_progress` — record a review result for a single card - `record_topic_progress` — update priority or resolve a backlog topic diff --git a/agents/shared/recording-protocol.md b/agents/shared/recording-protocol.md new file mode 100644 index 000000000..69bc9888a --- /dev/null +++ b/agents/shared/recording-protocol.md @@ -0,0 +1,64 @@ +# Recording Protocol — when the mentor writes + +The learning tier is fed by four **additive writers**. Each appends a record the +learner has agreed to or stated; none reschedules anything. This file is the one +instruction that says *when* each fires. Every harness definition points here, +so the mentor behaves the same on Kiro CLI, Claude Code, Codex, OpenCode, pi and +Grok Build — parity is of the instruction, never of approval: whether a call +prompts is the harness's own setting, and nothing here changes it. + +## Trigger table + +```yaml +- trigger: teach_back_agreed + when: "you proposed five rubric scores in one sentence and the learner's next reply accepted them (a yes, or a corrected set they say they accept)" + writer: record_teachback + required_ids: [concept, topic, scores, review_type] + consent: learner_agreed +- trigger: stuck_two_rounds + when: "two Socratic rounds on the same point without a breakthrough" + writer: log_struggle + required_ids: [question] + consent: learner_stated +- trigger: session_end_concepts + when: "the session is ending; one call per concept touched, with the status the learner confirmed" + writer: log_topic + required_ids: [topic, status] + consent: learner_confirmed +- trigger: plan_wind_down + when: "wind-down of a session run against an active study plan; one line on what was learned" + writer: record_plan_learning + required_ids: [plan_id, title] + consent: learner_agreed +``` + +## The triggers, one line each + +- **`teach_back_agreed` → `record_teachback`.** After a teach-back, propose the five scores + (accuracy, own words, structure, depth, transfer — each 1 to 4) in one sentence. When the + learner agrees, record them with the review type (`micro`, `structured`, `transfer`, `full`). + Not agreed, not recorded. A low-energy blank is never scored (teach-back protocol). +- **`stuck_two_rounds` → `log_struggle`.** When the learner has said, in effect, "I'm stuck" for + two rounds on one point, log the question so it returns for spaced repetition. Say that you did. +- **`session_end_concepts` → `log_topic`.** At wind-down, name each concept touched and the + status the learner confirms (`learning`, `struggling`, `insight`, `win`, `parked`) — one call each. +- **`plan_wind_down` → `record_plan_learning`.** If the session ran against an active plan, offer + one line for the plan's learning record; write it when the learner agrees. + +## What never fires from this table + +- `record_study_progress`, `log_review_outcome`, `record_topic_progress` reschedule a card or + resolve a backlog topic on an id nothing verifies. They stay learner-initiated, one prompt per + call, on every harness. +- A turn that matches no trigger writes nothing. Do not "keep the record warm". +- Nothing here reads a transcript or infers a score; the learner's agreement is the signal. + +## Saying so + +After any write, one short line: *"Recorded: window frame, structured, 15/20."* Then the next +question. Never a paragraph. + +If the write does not happen — the tool refuses, the learner declines the harness's approval prompt, +or the database is unavailable — say that in one line (*"Not recorded — the database refused; the +CLI `studyloop teachback …` is the fallback."*) and continue. Never retry silently, never claim a +record that did not land. diff --git a/agents/shared/teach-back-protocol.md b/agents/shared/teach-back-protocol.md index bc0d87bc9..457ceadf5 100644 --- a/agents/shared/teach-back-protocol.md +++ b/agents/shared/teach-back-protocol.md @@ -128,17 +128,23 @@ Flip the question orientation: ## Recording Teach-Back Scores -After each teach-back, record the score: - -```bash -studyloop teachback "" -t --score "3,3,4,3,2" --type structured --angle "apply_network_analogy" -``` - The agent should: 1. Assess the teach-back internally using the rubric 2. Propose the score to the student: "I'd score that as: Accuracy 3, Own Words 3, Structure 4, Depth 3, Transfer 2 — total 15/20. That's solid understanding with room on the transfer dimension. What do you think?" 3. Adjust if the student disagrees (metacognitive calibration) -4. Record the agreed score +4. Record the **agreed** score — and only an agreed one + +The agent records through the `record_teachback` MCP tool (trigger `teach_back_agreed` in +`agents/shared/recording-protocol.md`): concept, topic, the five scores in rubric order, the +review type, and optionally the angle and a note. The same rule on every harness; whether the +call prompts is the harness's own setting. Then one line — *"Recorded: window frame, structured, +15/20."* — and the next question. + +The CLI is the human's path to the same row, and validates identically: + +```bash +studyloop teachback "" -t --score "3,3,4,3,2" --type structured --angle "apply_network_analogy" +``` ### Score Transparency diff --git a/docs/architecture/learning-tier/council/brief-review9-2026-09-23.md b/docs/architecture/learning-tier/council/brief-review9-2026-09-23.md new file mode 100644 index 000000000..684e62377 --- /dev/null +++ b/docs/architecture/learning-tier/council/brief-review9-2026-09-23.md @@ -0,0 +1,1212 @@ +# Council brief — review 9: the mentor writes (learning tier, item 1, issue #38), PR #41, tree `3c175f78` + +You are one of three reviewers (different model families, answering independently). You are reviewing an +IMPLEMENTATION on branch `feat/learning-tier-fed` — 5 commits on top of `main` `c1a28de1` — as the tree to +merge to `main` for 0.5.x. The coordinator verifies every 🔴/🟡 finding against the repository before acting and +lands accepted corrections one commit each. Say UNVERIFIED rather than assume. Do not restate the brief. + +## 0. What you are reviewing against (binding) + +### The defect +The mentor agent can *read* learning state on every harness but records nothing: no MCP teach-back writer +existed (CLI only), the Kiro persona's only concrete "record progress" line pointed at `uv run tutor-checkpoint` +(a different tool), and no harness had a table saying WHEN to write. Measured: **zero writer calls in 143,973 +historical messages**. Owner verdict 2026-09-23: this is the MVP blocker — the failure that matters most in a +stranger's hands is "a mentor session whose teach-back never reached tomorrow's recommendation". + +### The plan and its owner decisions (binding, recorded in `docs/architecture/learning-tier/plan-2026-09-19.md` §7) +- **Decision 1 — two branches.** Item 1 (this tree) on `feat/learning-tier-fed`; item 2 (a Jev measurement) is + NOT on this branch and its branch is not cut until milestone 0.5.x closes. +- **Decision 2 — no new pre-approval on any harness.** The four additive writers (`W_auto` = `log_topic`, + `log_struggle`, `record_teachback` (new), `record_plan_learning`) are NAMED in every harness definition and stay + prompt-per-call wherever the harness prompts. Kiro's existing `allowedTools` entry for `log_topic` stays as the + ONE recorded asymmetry; Claude's `settings.json` gains no `permissions` block; OpenCode's `studyloop *` bash + wildcard is recorded as known and not least-privilege, unchanged. Parity is of the INSTRUCTION (one protocol, + one writer set), never of approval — Codex, pi and Grok Build have no in-repo grant grammar at all. +- **Decision 3 — the owner-led noticing episode** is scheduled by rule (first real study session after S1-GREEN + reaches `main`) and is an observation, not a gate. +- `W_srs` = `record_study_progress`, `log_review_outcome`, `record_topic_progress` reschedule a card or resolve a + backlog topic on an id nothing verifies; they stay prompt-per-call and no trigger names them. + +### Standing rules that bind this tree +- TDD: every RED committed failing for its stated reason before its GREEN; a RED expectation corrected at GREEN + is recorded in the GREEN commit and in the test, not erased. +- Six supported harnesses (kiro-cli, Claude Code, Codex, OpenCode, pi, Grok Build): nothing here may work for one + harness only. Grok Build reads the same canonical `agents/codex/AGENTS.md` (its adapter projects it). +- The learner's agreement is the signal: nothing infers a score from a transcript; a low-energy blank is never + scored (teach-back protocol's low-energy fallback). +- The MCP production inventory is pinned EXACTLY (32 → 33 here) in four places by design, so an accidental or + duplicate registration fails. +- Versioning rule (owner, 2026-09-21): stay on the 0.5.x patch line. + +### Facts you may rely on (verified on `3c175f78`, 2026-09-23) +- `history.teachback.record_teachback(concept, topic, scores, review_type, angle, notes, session_id) -> bool` + is the existing writer (used by the CLI); it inserts into `teach_back_scores` (five score columns each + `CHECK(... BETWEEN 1 AND 4)`), binds ownership via `agent_session_tools.context.records.bind`, records an + observation when available, and upserts `study_progress`. It returns `False` when no connection, and its + `except sqlite3.IntegrityError` path also returns `False`. +- `records.bind(..., session_id=...)` requires `session_id` to be a row of `sessions` visible in scope (a NATIVE + harness session) — `_owner()` raises `ScopeError("Source session is unavailable in the configured scope")` + otherwise; `study_session_id` links are permitted only for `parked_topics` and `study_notes`. This is why + **finding N1** below exists. +- The Kiro agent JSON (`agents/kiro/study-mentor.json`) exposes the whole `@studyloop` server under `tools` and + pre-approves five readers plus `@studyloop/log_topic` (`allowedTools:63-68`); `execute_bash` is also + pre-approved (`:52`), so the CLI writers already ran un-prompted there. +- Claude's sub-agent frontmatter (`agents/claude/socratic-mentor.md:5`) is the ONLY tool surface for that + sub-agent: an MCP tool not named on the `tools:` line is unreachable regardless of permissions. Before this tree + it read `tools: Read, Write, Grep, Bash`. +- `agents/manifest.json` hashes are `sha256(file bytes)[:16]`, regenerated by `scripts/update-agent-manifest.py` + over a tracked-file list; `test_install_agent_contracts.py::test_manifest_agent_entries_exist_and_match_installer_sources` + fails on any stale hash. +- Full unit suite on this tree: 7340 passed; failing ids minus the committed environmental set = the four + inventory pins (fixed in `3c175f78`) + `test_concatenated_remote_dump_with_existing_archive` (a pre-existing + `sqlite3 -bail` 60 s timeout in `agent-session-tools`, fails identically on clean `main`, recorded on PR #35). + `test_writer_isolation.py` passed on FIRST run, so per plan §5 it is a guard, not a RED. + +### Finding N1 (coordinator's own, corrected from the RED as first written — judge the correction) +The RED assumed the live study session id would land in `teach_back_scores.session_id`. The ownership layer +forbids it (fact above): the write failed and the tool reported "not recorded". GREEN therefore owns the row by +scope exactly as the CLI's rows are and returns `study_session_id` in the tool's REPLY only. The test +`test_the_study_session_is_reported_not_stored_as_the_rows_session_id` states this. Question for you: is +reporting-without-storing the right resolution, or should the tool bind the study session some other way that the +storage layer supports (e.g. the observation's `source_session_id`), and if so what is the concrete mechanism? + +## 1. Commits in the range (oldest first, c1a28de1..3c175f78) +- `fd70817c` docs(learning-tier): S1-0 capability-lock receipt — grants per harness from source, writer sets frozen +- `f439ab84` test(learning-tier): S1-RED — record_teachback tool, writers named everywhere, one recording protocol (22 red) +- `7354ea3a` feat(mcp): record_teachback tool through the validator the CLI now shares (finding N1 recorded) +- `0a22aabe` feat(agents): one recording protocol, named on every harness; the Kiro persona stops routing to tutor-checkpoint +- `3c175f78` test(mcp): production inventory pins move 32 -> 33 for record_teachback + +## What to return +Findings as 🔴 (must fix before merge) / 🟡 (should fix) / 🔵 (style) / 💡 (learning), each with file:line and a +one-line reason, then a verdict: ACCEPT / ACCEPT-WITH-CORRECTIONS / REJECT. Specifically judge: (a) whether the +trigger table is complete and its consent column is the right signal on every harness; (b) whether decision 2 is +honoured everywhere (names, no new grants); (c) N1; (d) whether any RED is vacuous or any GREEN over-claims; +(e) anything the S1-SIM stage (scripted learner across the six harnesses, owner-gated) must add before the claim +"pipe open on {passed}; plumbing proven for all six definitions" can be made. Prefix anything you did not verify +with UNVERIFIED. + +## 2. The recording protocol (verbatim, `agents/shared/recording-protocol.md`) + +````markdown +# Recording Protocol — when the mentor writes + +The learning tier is fed by four **additive writers**. Each appends a record the +learner has agreed to or stated; none reschedules anything. This file is the one +instruction that says *when* each fires. Every harness definition points here, +so the mentor behaves the same on Kiro CLI, Claude Code, Codex, OpenCode, pi and +Grok Build — parity is of the instruction, never of approval: whether a call +prompts is the harness's own setting, and nothing here changes it. + +## Trigger table + +```yaml +- trigger: teach_back_agreed + when: "a teach-back round has ended, you proposed five rubric scores, and the learner agreed them" + writer: record_teachback + required_ids: [concept, topic, scores, review_type] + consent: learner_agreed +- trigger: stuck_two_rounds + when: "two Socratic rounds on the same point without a breakthrough" + writer: log_struggle + required_ids: [question] + consent: learner_stated +- trigger: session_end_concepts + when: "the session is ending; one call per concept touched, with the status the learner confirmed" + writer: log_topic + required_ids: [topic, status] + consent: learner_confirmed +- trigger: plan_wind_down + when: "wind-down of a session run against an active study plan; one line on what was learned" + writer: record_plan_learning + required_ids: [plan_id, title] + consent: learner_agreed +``` + +## The triggers, one line each + +- **`teach_back_agreed` → `record_teachback`.** After a teach-back, propose the five scores + (accuracy, own words, structure, depth, transfer — each 1 to 4) in one sentence. When the + learner agrees, record them with the review type (`micro`, `structured`, `transfer`, `full`). + Not agreed, not recorded. A low-energy blank is never scored (teach-back protocol). +- **`stuck_two_rounds` → `log_struggle`.** When the learner has said, in effect, "I'm stuck" for + two rounds on one point, log the question so it returns for spaced repetition. Say that you did. +- **`session_end_concepts` → `log_topic`.** At wind-down, name each concept touched and the + status the learner confirms (`learning`, `struggling`, `insight`, `win`, `parked`) — one call each. +- **`plan_wind_down` → `record_plan_learning`.** If the session ran against an active plan, offer + one line for the plan's learning record; write it when the learner agrees. + +## What never fires from this table + +- `record_study_progress`, `log_review_outcome`, `record_topic_progress` reschedule a card or + resolve a backlog topic on an id nothing verifies. They stay learner-initiated, one prompt per + call, on every harness. +- A turn that matches no trigger writes nothing. Do not "keep the record warm". +- Nothing here reads a transcript or infers a score; the learner's agreement is the signal. + +## Saying so + +After any write, one short line: *"Recorded: window frame, structured, 15/20."* Then the next +question. Never a paragraph. +```` + +## 3. The S1-0 receipt (verbatim, `docs/architecture/learning-tier/receipts/s1-0-capability-lock.md`) + +````markdown +# S1-0 — Capability lock (learning tier, item 1) + +**Date:** 2026-09-23 · **Tree:** `main` @ `c1a28de1` (decisions recorded on PR #36, head `046fa499`) · +**Issue:** #38 · **Plan:** `docs/architecture/learning-tier/plan-2026-09-19.md` §3 S1-0 · **Branch:** `feat/learning-tier-fed` + +This receipt records, from source, how each of the six supported harnesses grants tool use to the +mentor today, and freezes the two writer sets item 1 builds against. It is the S1-0 finish line: +nothing here is a change; S1-RED and S1-GREEN cite it instead of re-deriving it. Every claim carries +the file and line it was read from on the tree above. + +## 1. Writer sets (frozen) + +| Set | Tools | Character | Rule this branch (owner decision 2, plan §7) | +|---|---|---|---| +| `W_auto` | `log_topic` (`mcp/tools.py:583`), `log_struggle` (`:850`), `record_plan_learning` (`:131`), **`record_teachback` (new — no MCP tool exists; the CLI writer is `history/teachback.py:27`)** | Additive: each appends a record the learner agreed to or stated | **Named in every harness definition; prompt-per-call everywhere; no new pre-approval on any harness.** | +| `W_srs` | `record_study_progress` (`mcp/tools.py:114`), `record_topic_progress` (`:538`), `log_review_outcome` (`:648`) | SRS mutators: reschedule a card or resolve a topic on an id nothing verifies (`card_hash`, backlog `topic_id`) | Prompt-per-call, unchanged, not named by any trigger. | + +Readers the definitions already name (`get_concept_context`, `get_study_history`, `get_next_action`, +`get_topic_suggestions`, `get_active_topics`, `read_lesson`, …) are outside both sets and unchanged. + +## 2. Grant mechanism per harness (as it is) + +| Harness | Definition file(s) | Grant grammar in repo | Approval cell today | S1-GREEN changes (instruction only) | +|---|---|---|---|---| +| **Kiro CLI** | `agents/kiro/study-mentor.json`; persona `agents/kiro/study-mentor/persona.md` | `tools` (`:13`) exposes `@builtin @study-speak @session-db @studyloop`; `allowedTools` (`:50-68`) pre-approves `execute_bash` (`:52`), five `@studyloop/get_*` readers (`:63-67`) and one writer, `@studyloop/log_topic` (`:68`) | `log_topic` pre-approved; the other three `W_auto` tools prompt; **CLI writers (`studyloop progress`, `studyloop teachback`) run un-prompted through `execute_bash`** | Persona repointed (`persona.md:32` and `:81` route "record progress" to `uv run tutor-checkpoint`, a different tool); trigger table projected. `allowedTools` **unchanged** — `log_topic` stays as the one recorded asymmetry. | +| **Claude Code** | `agents/claude/socratic-mentor.md`; `agents/claude/settings.json` | Sub-agent frontmatter `tools:` (`socratic-mentor.md:5`) = `Read, Write, Grep, Bash` — **no `mcp__studyloop__*` tool named**; `settings.json` has a single key, `statusLine` — **no `permissions` block**. The sibling `study-plan-architect.md:5` shows the naming grammar (ten `mcp__studyloop__*` tools). | Every tool call prompts; the mentor cannot call any `studyloop` MCP tool at all because none is named — it reaches the learning tier only through `Bash` and the CLI | `tools:` line names each `W_auto` tool and the readers (naming is instruction, not approval). **No `permissions` block is added.** | +| **OpenCode** | `agents/opencode/study-mentor.md` | `permission:` (`:10-16`): `edit: allow`; `bash:` `"studyloop *": allow`, `"session-* *": allow`, `"uv run tutor-*": allow`, `"*": ask` | CLI writers un-prompted through the bash wildcard (wider than least privilege — every `studyloop` verb); MCP tool approval is the harness default | Persona names each `W_auto` tool; **wildcard unchanged** (recorded here as known, not least-privilege, out of scope for this branch). | +| **Codex** | `agents/codex/AGENTS.md` (canonical persona, written into the session dir by `adapters/codex.py:20-27`) | **None in repo.** Codex reads `AGENTS.md` from cwd; approval is Codex's own policy | Harness-side | Canonical persona names each `W_auto` tool via the shared protocol; nothing else. | +| **pi** | `agents/pi/AGENTS.md` (+ `agents/pi/extensions/studyloop-session-export.ts`); written by `adapters/pi.py:15-16` | **None in repo.** | Harness-side | As Codex. | +| **Grok Build** | no `agents/grok/` directory (by design — `adapters/grok.py:97-101` writes `AGENTS.md` into the session dir; `:17` records that no Grok permission — `ui.yolo`, tool approval, hooks trust — is touched) | **None in repo.** | Harness-side | As Codex. No `agents/grok/` is created. | + +Parity, therefore, is of the **instruction** — one canonical `agents/shared/recording-protocol.md` with a +fenced YAML trigger table, projected byte-identically into every definition above and hashed in +`agents/manifest.json` — and of the **names**: every definition names the same `W_auto` set. It is never +parity of approval: three of the six harnesses cannot express approval in this repository at all. + +## 3. The writer that does not exist yet — its contract, read from the CLI + +`record_teachback` (MCP) must validate exactly as `cli/_teachback.py` does and land through the same +function: + +- Scores: exactly five integers, each 1–4 (`cli/_teachback.py:16-46`), in the order + `(accuracy, own_words, structure, depth, transfer)` (`history/teachback.py:27-42` docstring). +- `review_type` ∈ `TEACHBACK_TYPES = ("micro", "structured", "transfer", "full")` (`cli/_teachback.py:12`). +- `angle` and `notes` optional strings. +- Writer: `history.teachback.record_teachback(concept, topic, scores, review_type, angle, notes, + session_id) -> bool`; one row in `teach_back_scores`, whose five score columns carry + `CHECK(... BETWEEN 1 AND 4)` (`agent_session_tools/migrations.py:504-513`); `False` means no + connection, and the MCP tool must surface that as a `ToolError`, not a silent success. +- `session_id` is bound from the live session state, the way `end_session` reads it + (`mcp/tools.py:529-531`, `read_session_state()["study_session_id"]`), never from the caller. +- Teach-backs are events: a repeated call is two rows, documented as such (plan §5). + +## 4. What is *not* in this receipt, on purpose + +- No grant is widened or narrowed. The only edits S1-GREEN makes to definition files are names and the + projected protocol. +- The noticing episode (decision 3) is scheduled by rule — the first real study session after S1-GREEN + reaches `main` — and its date is written into the S1-SIM receipt, not here. +- Item 2 (Jev) is not on this branch (decision 1; #39). + +**S1-0 finish:** this file committed. Next: S1-RED (plan §5 table) — `test_mcp_teachback.py` (tool +absent), `test_adapter_parity.py` extended (names absent, no new grants), +`test_docs_harness_tier_contract.py` extended (protocol absent; `persona.md:32` still names +`tutor-checkpoint`), `test_writer_isolation.py` (guard if it already passes), no-trigger/duplicate replay. +```` + +## 4. Source, test and definition diffs — `c1a28de1..3c175f78`, in full (manifest and secrets baseline omitted: regenerated artefacts, see §0) + +```diff +diff --git a/agents/claude/socratic-mentor.md b/agents/claude/socratic-mentor.md +index d635c8ba..14ef15da 100644 +--- a/agents/claude/socratic-mentor.md ++++ b/agents/claude/socratic-mentor.md +@@ -2,7 +2,7 @@ + name: socratic-mentor + description: AuDHD-aware Socratic study mentor with spaced repetition, energy-adaptive sessions, network→DE concept bridges, and Clean Code/GoF discovery patterns + category: communication +-tools: Read, Write, Grep, Bash ++tools: Read, Write, Grep, Bash, mcp__studyloop__get_concept_context, mcp__studyloop__get_study_history, mcp__studyloop__get_next_action, mcp__studyloop__get_topic_suggestions, mcp__studyloop__get_active_topics, mcp__studyloop__log_topic, mcp__studyloop__log_struggle, mcp__studyloop__record_teachback, mcp__studyloop__record_plan_learning + --- + + # StudyLoop +@@ -19,6 +19,7 @@ See `agents/shared/knowledge-bridging.md` for configurable domain bridges. + See `agents/shared/break-science.md` for active break protocol. + See `agents/shared/wind-down-protocol.md` for end-of-session consolidation. + See `agents/shared/teach-back-protocol.md` for teach-back scoring. ++See `agents/shared/recording-protocol.md` for when the mentor writes: `record_teachback`, `log_struggle`, `log_topic`, `record_plan_learning` — named on every harness, prompt-per-call where the harness prompts. + + ## Identity + +diff --git a/agents/codex/AGENTS.md b/agents/codex/AGENTS.md +index a9551534..7ee213f7 100644 +--- a/agents/codex/AGENTS.md ++++ b/agents/codex/AGENTS.md +@@ -12,6 +12,7 @@ See `agents/shared/knowledge-bridging.md` for configurable domain bridges. + See `agents/shared/break-science.md` for active break protocol. + See `agents/shared/wind-down-protocol.md` for end-of-session consolidation. + See `agents/shared/teach-back-protocol.md` for teach-back scoring. ++See `agents/shared/recording-protocol.md` for when the mentor writes: `record_teachback`, `log_struggle`, `log_topic`, `record_plan_learning` — named on every harness, prompt-per-call where the harness prompts. + + ## Identity + +diff --git a/agents/kiro/study-mentor/persona.md b/agents/kiro/study-mentor/persona.md +index 0a4a249f..f047b400 100644 +--- a/agents/kiro/study-mentor/persona.md ++++ b/agents/kiro/study-mentor/persona.md +@@ -29,7 +29,7 @@ Follow the unified session protocol in `agents/shared/session-protocol.md`: + - One question at a time. Stop. Wait for response. + - Network→DE bridges for every new concept + - Max 3-4 concepts per explanation, TL;DR at top, mermaid diagrams for structure +-- Record progress: `uv run tutor-checkpoint --notes ""` ++- Record what the learner agreed, when `agents/shared/recording-protocol.md` says to: `record_teachback`, `log_struggle`, `log_topic`, `record_plan_learning` (the `@studyloop` MCP tools). Say so in one line, then the next question. + + ## Session Types + +@@ -76,10 +76,12 @@ studyloop review # What's due for review? + studyloop struggles # Recurring struggle topics + studyloop wins # Show learning wins + +-# Progress tracking ++# Progress tracking — the mentor writes through MCP (see recording-protocol.md): ++# @studyloop/record_teachback, @studyloop/log_struggle, @studyloop/log_topic, ++# @studyloop/record_plan_learning. The CLI below is the human's path to the same rows. + studyloop progress "" -t -c +-uv run tutor-checkpoint --notes "" +-session-query search "" # read back prior checkpoints for a skill ++studyloop teachback "" -t --score "3,3,4,3,2" --type structured ++session-query search "" # read back prior sessions for a skill + + # Cross-machine sync + studyloop state pull # Get latest from hub +@@ -100,3 +102,4 @@ Configured in `~/.config/studyloop/config.yaml` + - `agents/shared/audhd-framework.md` — Complete AuDHD cognitive support + - `agents/shared/socratic-engine.md` — Questioning methodology + - `agents/shared/network-bridges.md` — Network→DE concept bridges ++- `agents/shared/recording-protocol.md` — When the mentor writes (`record_teachback`, `log_struggle`, `log_topic`, `record_plan_learning`) +diff --git a/agents/mcp/README.md b/agents/mcp/README.md +index 1204aa09..d1fdb5de 100644 +--- a/agents/mcp/README.md ++++ b/agents/mcp/README.md +@@ -153,7 +153,7 @@ Requires a Google Cloud project with Calendar API enabled. See [setup guide](htt + + ## studyloop-mcp (Study tools) + +-The `studyloop-mcp` server exposes 32 MCP tools: courses and review cards, the study backlog and ++The `studyloop-mcp` server exposes 33 MCP tools: courses and review cards, the study backlog and + progress signals, lesson browsing, the live session, the `now` recommendation, and the learner's + study plans (nine lifecycle tools plus `record_plan_learning`, every one through the same plan + application layer the CLI and Web UI use — see `docs/agent-install.md`, "Study-plan tools over +@@ -199,6 +199,7 @@ server NAME is `studyloop`; `studyloop-mcp` is the console-script COMMAND, never + | `get_active_topics` | The AuDHD three-topic active set vs the remaining backlog | + | `log_topic` | Record a learning / struggling / insight signal mid-session | + | `log_struggle` | Record a topic the learner struggled with, for later study | ++| `record_teachback` | Record the five agreed teach-back scores for a concept — the mentor's write after a teach-back | + | `get_concept_context` | Concept dependency edges for a topic, with per-edge provenance and `coverage` — the prerequisite structure a mentor sequences from | + | `get_next_action` | The same "what now?" recommendation the web `/api/now` endpoint gives — plan-aware when a plan is active | + | `get_lesson_tree` | Browse the course-material tree: providers → courses → lessons | +diff --git a/agents/opencode/study-mentor.md b/agents/opencode/study-mentor.md +index da48f7e2..c1c865b6 100644 +--- a/agents/opencode/study-mentor.md ++++ b/agents/opencode/study-mentor.md +@@ -30,6 +30,7 @@ See `agents/shared/knowledge-bridging.md` for configurable domain bridges. + See `agents/shared/break-science.md` for active break protocol. + See `agents/shared/wind-down-protocol.md` for end-of-session consolidation. + See `agents/shared/teach-back-protocol.md` for teach-back scoring. ++See `agents/shared/recording-protocol.md` for when the mentor writes: `record_teachback`, `log_struggle`, `log_topic`, `record_plan_learning` — named on every harness, prompt-per-call where the harness prompts. + + ## Identity + +diff --git a/agents/pi/AGENTS.md b/agents/pi/AGENTS.md +index bf0f5bb8..9558858a 100644 +--- a/agents/pi/AGENTS.md ++++ b/agents/pi/AGENTS.md +@@ -12,6 +12,7 @@ See `~/.agents/shared/knowledge-bridging.md` for configurable domain bridges. + See `~/.agents/shared/break-science.md` for active break protocol. + See `~/.agents/shared/wind-down-protocol.md` for end-of-session consolidation. + See `~/.agents/shared/teach-back-protocol.md` for teach-back scoring. ++See `~/.agents/shared/recording-protocol.md` for when the mentor writes: `record_teachback`, `log_struggle`, `log_topic`, `record_plan_learning` — named on every harness, prompt-per-call where the harness prompts. + + ## Session Memory + +diff --git a/agents/shared/teach-back-protocol.md b/agents/shared/teach-back-protocol.md +index bc0d87bc..457ceadf 100644 +--- a/agents/shared/teach-back-protocol.md ++++ b/agents/shared/teach-back-protocol.md +@@ -128,17 +128,23 @@ Flip the question orientation: + + ## Recording Teach-Back Scores + +-After each teach-back, record the score: +- +-```bash +-studyloop teachback "" -t --score "3,3,4,3,2" --type structured --angle "apply_network_analogy" +-``` +- + The agent should: + 1. Assess the teach-back internally using the rubric + 2. Propose the score to the student: "I'd score that as: Accuracy 3, Own Words 3, Structure 4, Depth 3, Transfer 2 — total 15/20. That's solid understanding with room on the transfer dimension. What do you think?" + 3. Adjust if the student disagrees (metacognitive calibration) +-4. Record the agreed score ++4. Record the **agreed** score — and only an agreed one ++ ++The agent records through the `record_teachback` MCP tool (trigger `teach_back_agreed` in ++`agents/shared/recording-protocol.md`): concept, topic, the five scores in rubric order, the ++review type, and optionally the angle and a note. The same rule on every harness; whether the ++call prompts is the harness's own setting. Then one line — *"Recorded: window frame, structured, ++15/20."* — and the next question. ++ ++The CLI is the human's path to the same row, and validates identically: ++ ++```bash ++studyloop teachback "" -t --score "3,3,4,3,2" --type structured --angle "apply_network_analogy" ++``` + + ### Score Transparency + +diff --git a/docs/contributing.md b/docs/contributing.md +index 6577a5cd..d47376da 100644 +--- a/docs/contributing.md ++++ b/docs/contributing.md +@@ -125,7 +125,10 @@ harness's transcripts). + matter in six months, write an ADR (`docs/adr/NNNN-kebab-title.md`, indexed in `docs/adr/README.md`). + - *New mentor harness or backend*: the issue must define the persona mechanism, launch and resume + commands, session store, exporter, health check, failure behaviour and a live acceptance path. Drive-by +- harness additions are declined. ++ harness additions are declined. A mentor definition must also reference ++ `agents/shared/recording-protocol.md` and name its four writers (`record_teachback`, `log_struggle`, ++ `log_topic`, `record_plan_learning`) — `test_adapter_parity.py` and `test_docs_harness_tier_contract.py` ++ pin this, and pin that no harness gains a pre-approval for them (owner decision, 2026-09-23). + 2. **Fork and branch.** Outside contributors fork and open pull requests against `main`; maintainers branch + in the repository. Name branches `feat/…`, `fix/…`, `docs/…` or `test/…`. Do not use `lane/…`: that + prefix is reserved for the maintainers' remediation lanes and carries an ownership test that only makes +diff --git a/packages/studyloop/src/studyloop/cli/_teachback.py b/packages/studyloop/src/studyloop/cli/_teachback.py +index 04cc98a9..9eb2a00a 100644 +--- a/packages/studyloop/src/studyloop/cli/_teachback.py ++++ b/packages/studyloop/src/studyloop/cli/_teachback.py +@@ -8,12 +8,18 @@ import click + from rich.table import Table + + from studyloop.cli._shared import console ++from studyloop.history.teachback import TEACHBACK_TYPES, coerce_scores + +-TEACHBACK_TYPES = ("micro", "structured", "transfer", "full") ++__all__ = ["TEACHBACK_TYPES", "teachback"] + + + class TeachbackScoresParam(click.ParamType): +- """Parse five comma-separated teach-back rubric scores.""" ++ """Parse five comma-separated teach-back rubric scores. ++ ++ The rule itself lives in :func:`studyloop.history.teachback.coerce_scores`, ++ shared with the ``record_teachback`` MCP tool; this class only splits the ++ shell string and maps the shared ``ValueError`` to click's failure. ++ """ + + name = "scores" + +@@ -26,24 +32,10 @@ class TeachbackScoresParam(click.ParamType): + if isinstance(value, tuple): + return cast("tuple[int, int, int, int, int]", value) + +- raw = str(value) +- parts = [part.strip() for part in raw.split(",")] +- if len(parts) != 5 or any(part == "" for part in parts): +- self.fail( +- 'expected exactly five comma-separated scores, e.g. "3,3,4,3,2"', +- param, +- ctx, +- ) +- + try: +- scores = tuple(int(part) for part in parts) +- except ValueError: +- self.fail("scores must be integers from 1 to 4", param, ctx) +- +- if any(score < 1 or score > 4 for score in scores): +- self.fail("each score must be between 1 and 4", param, ctx) +- +- return cast("tuple[int, int, int, int, int]", scores) ++ return coerce_scores(str(value).split(",")) ++ except ValueError as exc: ++ self.fail(str(exc), param, ctx) + + + TEACHBACK_SCORES = TeachbackScoresParam() +diff --git a/packages/studyloop/src/studyloop/history/teachback.py b/packages/studyloop/src/studyloop/history/teachback.py +index a7141cc1..28b09aeb 100644 +--- a/packages/studyloop/src/studyloop/history/teachback.py ++++ b/packages/studyloop/src/studyloop/history/teachback.py +@@ -6,13 +6,57 @@ import json + import logging + import sqlite3 + from datetime import UTC, datetime ++from typing import TYPE_CHECKING, cast + + from agent_session_tools.context import records + + from . import _connection, observations + ++if TYPE_CHECKING: ++ from collections.abc import Sequence ++ + logger = logging.getLogger(__name__) + ++#: Review types a teach-back may be recorded under (CLI ``--type`` and MCP ``review_type``). ++TEACHBACK_TYPES = ("micro", "structured", "transfer", "full") ++ ++#: Rubric order of the five scores. ++SCORE_DIMENSIONS = ("accuracy", "own_words", "structure", "depth", "transfer") ++ ++_FIVE_SCORES = 'expected exactly five comma-separated scores, e.g. "3,3,4,3,2"' ++_INTEGERS = "scores must be integers from 1 to 4" ++_RANGE = "each score must be between 1 and 4" ++ ++ ++def coerce_scores(values: Sequence[object]) -> tuple[int, int, int, int, int]: ++ """Validate five rubric scores the one way both surfaces share. ++ ++ The CLI (``studyloop teachback --score``) and the MCP tool ++ (``record_teachback``) accept scores from different callers -- a shell ++ string and a JSON list -- but the rule is one: exactly five, integers, ++ each 1 to 4, in :data:`SCORE_DIMENSIONS` order. Raises ``ValueError`` ++ with the message the CLI has always printed; each surface maps that to ++ its own error type. ++ """ ++ parts = [str(part).strip() for part in values] ++ if len(parts) != 5 or any(part == "" for part in parts): ++ raise ValueError(_FIVE_SCORES) ++ try: ++ scores = tuple(int(part) for part in parts) ++ except ValueError as exc: ++ raise ValueError(_INTEGERS) from exc ++ if any(score < 1 or score > 4 for score in scores): ++ raise ValueError(_RANGE) ++ return cast("tuple[int, int, int, int, int]", scores) ++ ++ ++def coerce_review_type(value: str) -> str: ++ """Validate ``review_type`` against :data:`TEACHBACK_TYPES`; ``ValueError`` names the set.""" ++ if value not in TEACHBACK_TYPES: ++ allowed = ", ".join(TEACHBACK_TYPES) ++ raise ValueError(f"review type must be one of {allowed}; got {value!r}") ++ return value ++ + + def _confidence_from_teachback(total: int, review_type: str) -> str: + if total < 9: +diff --git a/packages/studyloop/src/studyloop/mcp/tools.py b/packages/studyloop/src/studyloop/mcp/tools.py +index e94d6b44..bdf8fa22 100644 +--- a/packages/studyloop/src/studyloop/mcp/tools.py ++++ b/packages/studyloop/src/studyloop/mcp/tools.py +@@ -864,6 +864,71 @@ def register_tools(mcp: FastMCP, *, include_exercises: bool = False) -> None: + row_id = park_topic(question, topic_tag=topic_tag, context=context, source="struggled") + return {"status": "logged", "id": row_id} + ++ @tool() ++ def record_teachback( ++ concept: str, ++ topic: str, ++ scores: list[int], ++ review_type: str, ++ angle: str = "", ++ notes: str = "", ++ ) -> dict[str, Any]: ++ """Record a teach-back score the learner has agreed to. ++ ++ Call once a teach-back round has ended and the learner has accepted ++ the five proposed scores (``agents/shared/recording-protocol.md``, ++ trigger ``teach_back_agreed``). Scores are in rubric order -- ++ accuracy, own_words, structure, depth, transfer -- each 1 to 4; ++ ``review_type`` is one of micro, structured, transfer, full. ++ Teach-backs are events: a repeated call records a second row. ++ ++ The row is owned by the configured scope, exactly as the CLI's rows ++ are. The live study session id is returned for the caller's record ++ but is not a storage link: the ownership layer binds a ``session_id`` ++ only to a native harness session and links study sessions only for ++ parked topics and study notes (S1-0 receipt, finding N1). ++ ++ Args: ++ concept: The concept that was taught back. ++ topic: Study topic (python, sql, ...). ++ scores: Exactly five integers, each 1-4, in rubric order. ++ review_type: micro, structured, transfer or full. ++ angle: Optional question angle used (e.g. "apply_network_analogy"). ++ notes: Optional assessment notes. ++ """ ++ from studyloop.history.teachback import coerce_review_type, coerce_scores ++ from studyloop.history.teachback import record_teachback as write_teachback ++ from studyloop.session_state import read_session_state ++ ++ try: ++ five = coerce_scores(scores) ++ kind = coerce_review_type(review_type) ++ except ValueError as exc: ++ raise ToolError(str(exc)) from exc ++ ++ study_session_id = read_session_state().get("study_session_id") or None ++ recorded = write_teachback( ++ concept=concept, ++ topic=topic, ++ scores=five, ++ review_type=kind, ++ angle=angle or None, ++ notes=notes or None, ++ ) ++ if not recorded: ++ raise ToolError( ++ "teach-back not recorded: the sessions database is unavailable " ++ "(run `studyloop doctor`)" ++ ) ++ return { ++ "recorded": True, ++ "concept": concept, ++ "topic": topic, ++ "review_type": kind, ++ "total": sum(five), ++ "study_session_id": study_session_id, ++ } ++ + # ── Study plans — discovery, authoring and progression through the seam (D-4, D-8, D-9) ── + # + # Nine thin adapters over ``studyloop.planning.PlanApplication`` (design §4): +diff --git a/packages/studyloop/tests/test_adapter_parity.py b/packages/studyloop/tests/test_adapter_parity.py +index 29f41274..b49af332 100644 +--- a/packages/studyloop/tests/test_adapter_parity.py ++++ b/packages/studyloop/tests/test_adapter_parity.py +@@ -90,3 +90,86 @@ def test_readme_names_every_supported_harness_by_its_label() -> None: + if not re.search(rf"\b{re.escape(harness.label)}\b", text) + ] + assert not missing, f"README.md does not name these supported harnesses: {missing}" ++ ++ ++# --------------------------------------------------------------------------- ++# Learning tier, item 1 (S1-RED): the writers are NAMED everywhere, GRANTED nowhere new ++# --------------------------------------------------------------------------- ++# ++# Owner decision 2 (docs/architecture/learning-tier/plan-2026-09-19.md §7): the ++# additive writers are named in every harness definition and stay prompt-per-call; ++# no harness gains a pre-approval on this branch. Parity is of the instruction, ++# never of approval -- codex, pi and grok cannot express approval in this repo. ++# The S1-0 receipt (docs/architecture/learning-tier/receipts/s1-0-capability-lock.md) ++# records the cells these tests pin. ++ ++#: The additive, learner-agreed writers (``record_teachback`` is the new one). ++W_AUTO = ("log_topic", "log_struggle", "record_teachback", "record_plan_learning") ++ ++#: The mentor definition each harness actually loads. Grok Build reads the same ++#: canonical file as Codex (its header says so; ``adapters/grok.py`` projects it). ++MENTOR_DEFINITIONS = { ++ "kiro": "agents/kiro/study-mentor/persona.md", ++ "claude": "agents/claude/socratic-mentor.md", ++ "opencode": "agents/opencode/study-mentor.md", ++ "codex": "agents/codex/AGENTS.md", ++ "pi": "agents/pi/AGENTS.md", ++ "grok": "agents/codex/AGENTS.md", ++} ++ ++ ++def _definition(harness: str) -> str: ++ return (REPO_ROOT / MENTOR_DEFINITIONS[harness]).read_text(encoding="utf-8") ++ ++ ++def test_mentor_definitions_cover_exactly_the_six() -> None: ++ assert set(MENTOR_DEFINITIONS) == EXPECTED_HARNESSES ++ ++ ++def test_every_mentor_definition_names_each_w_auto_writer() -> None: ++ """A writer the definition never names is a writer the mentor never calls.""" ++ missing = { ++ harness: [w for w in W_AUTO if not re.search(rf"\b{w}\b", _definition(harness))] ++ for harness in sorted(MENTOR_DEFINITIONS) ++ } ++ missing = {h: ws for h, ws in missing.items() if ws} ++ assert not missing, f"W_auto writers not named: {missing}" ++ ++ ++def test_claude_mentor_tools_line_names_each_writer() -> None: ++ """Claude's sub-agent frontmatter restricts tools to the ``tools:`` line; ++ an unnamed MCP tool is unreachable there, whatever the permissions say.""" ++ text = _definition("claude") ++ match = re.search(r"^tools:\s*(.+)$", text, flags=re.MULTILINE) ++ assert match, "socratic-mentor.md has no frontmatter tools: line" ++ named = {item.strip() for item in match.group(1).split(",")} ++ missing = [w for w in W_AUTO if f"mcp__studyloop__{w}" not in named] ++ assert not missing, f"tools: line does not name {missing}; it names {sorted(named)}" ++ ++ ++def test_kiro_gains_no_pre_approval() -> None: ++ """The one recorded asymmetry stays exactly one: ``log_topic``.""" ++ import json ++ ++ spec = json.loads((REPO_ROOT / "agents/kiro/study-mentor.json").read_text(encoding="utf-8")) ++ allowed = set(spec["allowedTools"]) ++ assert allowed & {f"@studyloop/{w}" for w in W_AUTO} == {"@studyloop/log_topic"} ++ ++ ++def test_claude_settings_grant_nothing() -> None: ++ import json ++ ++ settings = json.loads((REPO_ROOT / "agents/claude/settings.json").read_text(encoding="utf-8")) ++ assert "permissions" not in settings, "decision 2: no permissions block on Claude" ++ ++ ++def test_opencode_permission_block_is_exactly_as_recorded() -> None: ++ """Recorded as known and not least-privilege in the S1-0 receipt; unchanged here.""" ++ text = _definition("opencode") ++ for line in ( ++ '"studyloop *": allow', ++ '"session-* *": allow', ++ '"uv run tutor-*": allow', ++ '"*": ask', ++ ): ++ assert line in text, f"opencode permission block changed: {line!r} missing" +diff --git a/packages/studyloop/tests/test_docs_harness_tier_contract.py b/packages/studyloop/tests/test_docs_harness_tier_contract.py +index 07d3ea88..4bc81f2c 100644 +--- a/packages/studyloop/tests/test_docs_harness_tier_contract.py ++++ b/packages/studyloop/tests/test_docs_harness_tier_contract.py +@@ -18,6 +18,7 @@ from __future__ import annotations + + import re + from pathlib import Path ++from typing import ClassVar + + from studyloop.harnesses import CORE_HARNESSES, HARNESSES, PREVIEW_HARNESSES, RELEASE_HARNESSES + +@@ -162,3 +163,80 @@ class TestHarnessRecordsAgreeWithTiers: + def test_core_flag_mirrors_the_tuples(self) -> None: + assert {n for n, h in HARNESSES.items() if h.core} == set(CORE_HARNESSES) + assert {n for n, h in HARNESSES.items() if not h.core} == set(PREVIEW_HARNESSES) ++ ++ ++# --------------------------------------------------------------------------- ++# Learning tier, item 1 (S1-RED): one recording protocol, projected to every harness ++# --------------------------------------------------------------------------- ++ ++ ++class TestRecordingProtocol: ++ """``agents/shared/recording-protocol.md`` is the one instruction that tells ++ every mentor WHEN to write. Plan §5; owner decision 2 (parity of instruction). ++ """ ++ ++ PROTOCOL = "agents/shared/recording-protocol.md" ++ W_AUTO = ("log_topic", "log_struggle", "record_teachback", "record_plan_learning") ++ DEFINITIONS = ( ++ "agents/kiro/study-mentor/persona.md", ++ "agents/claude/socratic-mentor.md", ++ "agents/opencode/study-mentor.md", ++ "agents/codex/AGENTS.md", ++ "agents/pi/AGENTS.md", ++ ) ++ REQUIRED_KEYS: ClassVar[frozenset[str]] = frozenset( ++ {"trigger", "writer", "required_ids", "consent"} ++ ) ++ ++ def _table(self) -> list[dict]: ++ import yaml ++ ++ text = _read(self.PROTOCOL) ++ blocks = re.findall(r"^```yaml\s*\n(.*?)^```", text, flags=re.MULTILINE | re.DOTALL) ++ assert len(blocks) == 1, f"expected exactly one fenced yaml block, found {len(blocks)}" ++ table = yaml.safe_load(blocks[0]) ++ assert isinstance(table, list) and table, "the trigger table is a non-empty YAML list" ++ return table ++ ++ def test_the_protocol_exists(self) -> None: ++ assert (REPO_ROOT / self.PROTOCOL).is_file(), f"{self.PROTOCOL} is absent" ++ ++ def test_the_trigger_table_parses_with_the_documented_columns(self) -> None: ++ for row in self._table(): ++ assert set(row) >= self.REQUIRED_KEYS, ( ++ f"row lacks {self.REQUIRED_KEYS - set(row)}: {row}" ++ ) ++ ++ def test_every_trigger_names_a_w_auto_writer_and_every_writer_has_a_trigger(self) -> None: ++ writers = [row["writer"] for row in self._table()] ++ strangers = [w for w in writers if w not in self.W_AUTO] ++ assert not strangers, f"triggers name writers outside W_auto: {strangers}" ++ untriggered = [w for w in self.W_AUTO if w not in writers] ++ assert not untriggered, f"W_auto writers with no trigger: {untriggered}" ++ ++ def test_no_trigger_names_an_srs_mutator(self) -> None: ++ srs = {"record_study_progress", "log_review_outcome", "record_topic_progress"} ++ assert not {row["writer"] for row in self._table()} & srs ++ ++ def test_every_mentor_definition_references_the_protocol(self) -> None: ++ missing = [d for d in self.DEFINITIONS if "recording-protocol.md" not in _read(d)] ++ assert not missing, f"definitions that do not reference the protocol: {missing}" ++ ++ def test_the_manifest_hashes_the_protocol(self) -> None: ++ import hashlib ++ import json ++ ++ manifest = json.loads(_read("agents/manifest.json")) ++ entry = manifest["agents"].get("shared/recording-protocol.md") ++ assert entry, "agents/manifest.json has no entry for shared/recording-protocol.md" ++ digest = hashlib.sha256((REPO_ROOT / self.PROTOCOL).read_bytes()).hexdigest()[:16] ++ assert entry["hash"] == digest, "manifest hash is stale for the protocol" ++ ++ def test_the_kiro_persona_no_longer_routes_record_progress_to_tutor_checkpoint(self) -> None: ++ lines = _read("agents/kiro/study-mentor/persona.md").splitlines() ++ offenders = [ ++ ln for ln in lines if "record progress" in ln.lower() and "tutor-checkpoint" in ln ++ ] ++ assert offenders == [], ( ++ f"persona still routes record-progress to tutor-checkpoint: {offenders}" ++ ) +diff --git a/packages/studyloop/tests/test_mcp_plan_tools.py b/packages/studyloop/tests/test_mcp_plan_tools.py +index f37ddd99..57b93e0b 100644 +--- a/packages/studyloop/tests/test_mcp_plan_tools.py ++++ b/packages/studyloop/tests/test_mcp_plan_tools.py +@@ -97,8 +97,9 @@ NINE_TOOLS: tuple[str, ...] = SIX_TOOLS + PHASE_FOUR_TOOLS + #: 23 original tools at ``0a20a796`` — ``record_plan_learning`` among them — + #: plus the nine design-§4 plan tools = 32 (council review 3, F13: the design's + #: "35" was arithmetic on a stale inventory; review 4, F6: the earlier comment +-#: here said "plus nine less one", which is 31). +-PRODUCTION_TOOL_COUNT = 32 ++#: here said "plus nine less one", which is 31), plus ``record_teachback`` ++#: (learning tier item 1, 2026-09-23) = 33. ++PRODUCTION_TOOL_COUNT = 33 + + #: The core names the stdio smoke test also pins; asserted here too so the + #: in-process twin is a real twin (review 4, F4). +@@ -933,8 +934,8 @@ def test_phase_four_schemas_carry_the_design_signatures() -> None: + assert "const" not in confirmed and "enum" not in confirmed + + +-def test_production_inventory_is_thirty_two_with_the_nine_plan_tools() -> None: +- """The in-process twin of the stdio pin (T4.1): exactly 32 names, each ++def test_production_inventory_is_thirty_three_with_the_nine_plan_tools() -> None: ++ """The in-process twin of the stdio pin (T4.1): exactly 33 names, each + tool registered under its own name (a duplicate registration would + overwrite its key silently, so the dict's size alone cannot show one), + all nine design-§4 plan tools, ``record_plan_learning`` and the core +diff --git a/packages/studyloop/tests/test_mcp_stdio_smoke.py b/packages/studyloop/tests/test_mcp_stdio_smoke.py +index 1490a597..b9561fd5 100644 +--- a/packages/studyloop/tests/test_mcp_stdio_smoke.py ++++ b/packages/studyloop/tests/test_mcp_stdio_smoke.py +@@ -42,10 +42,11 @@ PLAN_TOOLS = { + #: ``record_plan_learning`` among them — plus the nine plan tools = 32 + #: (council review 3, F13 — the design's "26 → 35" was arithmetic on a stale + #: count; review 4, F6 — an earlier form of this comment said "plus nine less +-#: one", which is 31). ++#: one", which is 31), plus ``record_teachback`` (learning tier item 1, ++#: 2026-09-23) = 33. + #: Exact, not a lower bound: an accidental registration is a failure here, and + #: the name assertions stop an unrelated addition masking a missing tool. +-PRODUCTION_TOOL_COUNT = 32 ++PRODUCTION_TOOL_COUNT = 33 + + + @pytest.fixture +diff --git a/packages/studyloop/tests/test_mcp_teachback.py b/packages/studyloop/tests/test_mcp_teachback.py +new file mode 100644 +index 00000000..eed5e6a2 +--- /dev/null ++++ b/packages/studyloop/tests/test_mcp_teachback.py +@@ -0,0 +1,215 @@ ++"""RED for the ``record_teachback`` MCP tool (learning tier, item 1, stage S1-RED). ++ ++The learning tier is unfed because the mentor has no MCP writer for a teach-back ++score: the only writer is the CLI (``studyloop teachback``), which an agent ++session never calls. Plan §5 (``docs/architecture/learning-tier/plan-2026-09-19.md``) ++and the S1-0 receipt pin the contract this file tests: ++ ++* the tool exists beside the other ``W_auto`` writers; ++* it validates exactly as ``cli/_teachback.py`` does -- five scores, integers, ++ each 1-4, ``review_type`` in ``TEACHBACK_TYPES`` -- and lands **no row** when ++ validation fails; ++* a valid call lands **one row** in ``teach_back_scores`` through the real ++ migrated schema, whose CHECK constraints are what "honouring CHECK" means; ++* the live study session id is reported in the reply and never taken from ++ the caller; it is not stored as the row's ``session_id`` (finding N1: the ++ ownership layer reserves that column for native harness sessions); ++* a repeated call is two rows -- teach-backs are events, not state; ++* a missing connection is a ``ToolError``, never a silent success. ++ ++Accesses the tool via the FastMCP registry, mirroring ``test_mcp_log_topic.py``. ++The scratch database comes from ``STUDYLOOP_DB`` (read at call time), so the ++schema is the one ``_connection._connect`` migrates, not a hand-rolled table. ++""" ++ ++from __future__ import annotations ++ ++import inspect ++import sqlite3 ++from typing import TYPE_CHECKING ++ ++import pytest ++ ++pytest.importorskip("mcp") ++ ++from mcp.server.fastmcp.exceptions import ToolError ++ ++from studyloop.mcp.server import mcp ++ ++if TYPE_CHECKING: ++ from pathlib import Path ++ ++W_AUTO = ("log_topic", "log_struggle", "record_teachback", "record_plan_learning") ++VALID_SCORES = [3, 3, 4, 3, 2] ++ ++ ++def _get_tool(name: str): ++ tools = mcp._tool_manager._tools ++ if name not in tools: ++ raise KeyError(f"Tool {name!r} not found. Available: {sorted(tools)}") ++ return tools[name].fn ++ ++ ++def _rows(db: Path) -> list[tuple]: ++ conn = sqlite3.connect(db) ++ try: ++ return conn.execute( ++ "SELECT concept, topic, session_id, score_accuracy, score_own_words, " ++ "score_structure, score_depth, score_transfer FROM teach_back_scores ORDER BY id" ++ ).fetchall() ++ except sqlite3.OperationalError as exc: # table absent: nothing was ever written ++ if "no such table" in str(exc): ++ return [] ++ raise ++ finally: ++ conn.close() ++ ++ ++@pytest.fixture() ++def scratch_db(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: ++ """A per-test sessions.db that the real connection resolver migrates.""" ++ db = tmp_path / "sessions.db" ++ monkeypatch.setenv("STUDYLOOP_DB", str(db)) ++ return db ++ ++ ++@pytest.fixture() ++def session_state(tmp_path: Path, monkeypatch: pytest.MonkeyPatch): ++ """Redirect the session-state files into tmp_path and return the writer.""" ++ from studyloop import session_state as ss ++ ++ monkeypatch.setattr(ss, "SESSION_DIR", tmp_path) ++ monkeypatch.setattr(ss, "STATE_FILE", tmp_path / "session-state.json") ++ monkeypatch.setattr(ss, "TOPICS_FILE", tmp_path / "session-topics.md") ++ monkeypatch.setattr(ss, "PARKING_FILE", tmp_path / "session-parking.md") ++ return ss.write_session_state ++ ++ ++class TestTheToolExists: ++ def test_record_teachback_is_registered_beside_the_other_writers(self) -> None: ++ registered = set(mcp._tool_manager._tools) ++ missing = [name for name in W_AUTO if name not in registered] ++ assert missing == [], f"W_auto writers absent from the MCP registry: {missing}" ++ ++ def test_the_caller_cannot_supply_a_session_id(self) -> None: ++ params = inspect.signature(_get_tool("record_teachback")).parameters ++ assert "session_id" not in params, "session_id is bound from session state, not the caller" ++ ++ ++class TestValidationMirrorsTheCli: ++ """Every rejection is a ToolError and leaves the table untouched.""" ++ ++ @pytest.mark.parametrize( ++ ("scores", "fragment"), ++ [ ++ ([3, 3, 4, 3], "five"), ++ ([3, 3, 4, 3, 2, 1], "five"), ++ ([0, 3, 4, 3, 2], "1 and 4"), ++ ([3, 3, 4, 3, 5], "1 and 4"), ++ ], ++ ) ++ def test_score_shape_and_range(self, scratch_db: Path, scores: list, fragment: str) -> None: ++ tool = _get_tool("record_teachback") ++ with pytest.raises(ToolError, match=fragment): ++ tool(concept="window frame", topic="sql", scores=scores, review_type="micro") ++ assert _rows(scratch_db) == [] ++ ++ def test_non_integer_scores_are_rejected(self, scratch_db: Path) -> None: ++ tool = _get_tool("record_teachback") ++ with pytest.raises(ToolError, match="integer"): ++ tool( ++ concept="window frame", topic="sql", scores=["3", "x", 4, 3, 2], review_type="micro" ++ ) ++ assert _rows(scratch_db) == [] ++ ++ def test_unknown_review_type_is_rejected_naming_the_allowed_set(self, scratch_db: Path) -> None: ++ from studyloop.cli._teachback import TEACHBACK_TYPES ++ ++ tool = _get_tool("record_teachback") ++ with pytest.raises(ToolError) as excinfo: ++ tool(concept="window frame", topic="sql", scores=VALID_SCORES, review_type="vibes") ++ for allowed in TEACHBACK_TYPES: ++ assert allowed in str(excinfo.value) ++ assert _rows(scratch_db) == [] ++ ++ ++class TestARowLands: ++ def test_a_valid_call_lands_exactly_one_row_through_the_real_schema( ++ self, scratch_db: Path, session_state ++ ) -> None: ++ session_state({"study_session_id": "study-42"}) ++ tool = _get_tool("record_teachback") ++ ++ result = tool( ++ concept="window frame", ++ topic="sql", ++ scores=VALID_SCORES, ++ review_type="structured", ++ angle="apply_network_analogy", ++ notes="explained ROWS vs RANGE unprompted", ++ ) ++ ++ assert result["recorded"] is True ++ assert result["total"] == sum(VALID_SCORES) ++ rows = _rows(scratch_db) ++ assert rows == [("window frame", "sql", None, 3, 3, 4, 3, 2)] ++ # The schema is the migrated one: its CHECK constraints are present. ++ conn = sqlite3.connect(scratch_db) ++ try: ++ ddl = conn.execute( ++ "SELECT sql FROM sqlite_master WHERE type='table' AND name='teach_back_scores'" ++ ).fetchone()[0] ++ finally: ++ conn.close() ++ assert "BETWEEN 1 AND 4" in ddl ++ ++ def test_the_study_session_is_reported_not_stored_as_the_rows_session_id( ++ self, scratch_db: Path, session_state ++ ) -> None: ++ """Finding N1 (S1-GREEN, corrected from the RED as first written). ++ ++ The RED assumed the live study session id would land in the row's ++ ``session_id``. The ownership layer forbids it: ``records.bind`` treats ++ ``session_id`` as a *native* harness session (it must exist in ++ ``sessions`` and be visible in scope) and links study sessions only for ++ ``parked_topics`` and ``study_notes`` -- the write failed and the tool ++ reported "not recorded". So the row is owned by scope, as the CLI's ++ rows are, and the study session id travels in the tool's reply. ++ """ ++ session_state({"study_session_id": "study-7"}) ++ result = _get_tool("record_teachback")( ++ concept="decorators", topic="python", scores=VALID_SCORES, review_type="micro" ++ ) ++ assert result["study_session_id"] == "study-7" ++ assert [row[2] for row in _rows(scratch_db)] == [None] ++ ++ def test_no_live_session_reports_none_and_still_records( ++ self, scratch_db: Path, session_state ++ ) -> None: ++ result = _get_tool("record_teachback")( ++ concept="decorators", topic="python", scores=VALID_SCORES, review_type="micro" ++ ) ++ assert result["study_session_id"] is None ++ assert len(_rows(scratch_db)) == 1 ++ ++ def test_a_repeated_call_is_two_rows_because_teachbacks_are_events( ++ self, scratch_db: Path, session_state ++ ) -> None: ++ tool = _get_tool("record_teachback") ++ for _ in range(2): ++ tool(concept="decorators", topic="python", scores=VALID_SCORES, review_type="micro") ++ assert len(_rows(scratch_db)) == 2 ++ ++ ++class TestFailureIsLoud: ++ def test_no_connection_is_a_tool_error_not_a_silent_success( ++ self, scratch_db: Path, monkeypatch: pytest.MonkeyPatch ++ ) -> None: ++ import studyloop.history._connection as _conn ++ ++ monkeypatch.setattr(_conn, "_connect", lambda: None) ++ with pytest.raises(ToolError, match="not recorded"): ++ _get_tool("record_teachback")( ++ concept="decorators", topic="python", scores=VALID_SCORES, review_type="micro" ++ ) ++ assert _rows(scratch_db) == [] +diff --git a/packages/studyloop/tests/test_verify_plan_integration_script.py b/packages/studyloop/tests/test_verify_plan_integration_script.py +index 6e8b7ab7..8ecfd723 100644 +--- a/packages/studyloop/tests/test_verify_plan_integration_script.py ++++ b/packages/studyloop/tests/test_verify_plan_integration_script.py +@@ -464,11 +464,11 @@ class TestRealPythonChecks: + assert code == 0, measured + assert measured["sha256"] == script.GOLDEN_SHA256 + +- def test_inventory_check_reports_thirty_two_names_with_the_nine(self, script) -> None: ++ def test_inventory_check_reports_thirty_three_names_with_the_nine(self, script) -> None: + code, measured = script.check_inventory_in_process(REPO_ROOT) + assert code == 0, measured +- assert measured["count"] == 32 +- assert len(measured["names"]) == len(set(measured["names"])) == 32 ++ assert measured["count"] == 33 ++ assert len(measured["names"]) == len(set(measured["names"])) == 33 + assert set(script.PLAN_TOOL_NAMES) <= set(measured["names"]) + assert "record_plan_learning" in measured["names"] + +diff --git a/packages/studyloop/tests/test_writer_isolation.py b/packages/studyloop/tests/test_writer_isolation.py +new file mode 100644 +index 00000000..001ef5b0 +--- /dev/null ++++ b/packages/studyloop/tests/test_writer_isolation.py +@@ -0,0 +1,119 @@ ++"""Writer isolation (learning tier, item 1, plan §5 "Isolation"). ++ ++Every ``W_auto`` writer is driven in a **child process** whose ``HOME`` and ++``XDG_*`` point at a decoy tree while the four StudyLoop redirects point at a ++sandbox. If any writer resolved a path through the home directory instead of ++the redirect, a test run would reach the learner's real database or session ++files -- the exact failure ``conftest.py`` documents from 2026-09-05. The ++child writes through the real MCP tool functions, then the parent asserts: ++ ++* the decoy home tree holds no new file (including Markdown); ++* each writer's row or line landed inside the sandbox. ++ ++Plan §5 says this test is a guard if it already passes on ``main`` and RED ++only if it fails; it passed on first run, so it is the guard. ++""" ++ ++from __future__ import annotations ++ ++import json ++import os ++import subprocess ++import sys ++import textwrap ++from typing import TYPE_CHECKING ++ ++import pytest ++ ++if TYPE_CHECKING: ++ from pathlib import Path ++ ++pytest.importorskip("mcp") ++ ++_CHILD = textwrap.dedent( ++ """ ++ import json, os, sys ++ from studyloop.mcp.server import mcp ++ tools = mcp._tool_manager._tools ++ ++ def call(name, **kw): ++ return tools[name].fn(**kw) ++ ++ out = {} ++ out["log_topic"] = call("log_topic", topic="window frame", status="learning", note="iso") ++ out["log_struggle"] = call("log_struggle", question="why ROWS not RANGE", topic_tag="sql") ++ out["record_teachback"] = call( ++ "record_teachback", concept="window frame", topic="sql", ++ scores=[3, 3, 4, 3, 2], review_type="micro", ++ ) ++ plan = call( ++ "create_study_plan", title="Isolation plan", ++ answers={"why": "prove the writers stay in the sandbox", "success": ["one row each"], ++ "topics": ["sql"], "milestones": [{"title": "Frames"}]}, ++ plan_id="isolation-plan", ++ ) ++ out["record_plan_learning"] = call( ++ "record_plan_learning", plan_id="isolation-plan", title="Frames are ROWS or RANGE", ++ body="isolation body", ++ ) ++ print(json.dumps(out, default=str)) ++ """ ++) ++ ++ ++def _tree(root: Path) -> set[str]: ++ return {str(p.relative_to(root)) for p in root.rglob("*") if p.is_file()} ++ ++ ++def test_every_w_auto_writer_stays_inside_the_sandbox(tmp_path: Path) -> None: ++ decoy_home = tmp_path / "decoy-home" ++ sandbox = tmp_path / "sandbox" ++ for d in (decoy_home, sandbox / "state", sandbox / "session", sandbox / "plans"): ++ d.mkdir(parents=True) ++ # A decoy config dir that looks like a real one, so a writer that resolves ++ # through HOME has somewhere plausible to land. ++ (decoy_home / ".config" / "studyloop").mkdir(parents=True) ++ (decoy_home / ".local" / "share" / "studyloop").mkdir(parents=True) ++ before = _tree(decoy_home) ++ ++ env = { ++ k: v ++ for k, v in os.environ.items() ++ if not k.startswith(("STUDYLOOP_", "XDG_")) and k != "HOME" ++ } ++ env.update( ++ { ++ "HOME": str(decoy_home), ++ "XDG_CONFIG_HOME": str(decoy_home / ".config"), ++ "XDG_DATA_HOME": str(decoy_home / ".local" / "share"), ++ "STUDYLOOP_DB": str(sandbox / "sessions.db"), ++ "STUDYLOOP_STATE_DIR": str(sandbox / "state"), ++ "STUDYLOOP_SESSION_DIR": str(sandbox / "session"), ++ "STUDYLOOP_PLANS_DIR": str(sandbox / "plans"), ++ "STUDYLOOP_CONFIG": str(sandbox / "config.yaml"), ++ } ++ ) ++ proc = subprocess.run( ++ [sys.executable, "-c", _CHILD], ++ env=env, ++ capture_output=True, ++ text=True, ++ timeout=120, ++ check=False, ++ ) ++ assert proc.returncode == 0, proc.stderr[-2000:] ++ out = json.loads(proc.stdout.strip().splitlines()[-1]) ++ ++ # Nothing escaped to the decoy home -- not a database, not a Markdown file. ++ escaped = _tree(decoy_home) - before ++ assert escaped == set(), f"writers reached the home tree: {sorted(escaped)}" ++ ++ # Each writer landed inside the sandbox. ++ assert out["record_teachback"]["recorded"] is True ++ assert out["record_plan_learning"]["created"] is True ++ assert out["log_struggle"]["status"] == "logged" ++ assert (sandbox / "session" / "session-topics.md").is_file(), "log_topic wrote no session line" ++ assert (sandbox / "sessions.db").is_file(), "record_teachback / log_struggle wrote no database" ++ plan_docs = list((sandbox / "plans").rglob("*.md")) ++ assert plan_docs, "record_plan_learning wrote no plan document in the sandbox" ++ assert any("Frames are ROWS or RANGE" in p.read_text(encoding="utf-8") for p in plan_docs) +diff --git a/scripts/update-agent-manifest.py b/scripts/update-agent-manifest.py +index 60bcea30..7aa71e5b 100644 +--- a/scripts/update-agent-manifest.py ++++ b/scripts/update-agent-manifest.py +@@ -42,6 +42,7 @@ SHARED_FILES = [ + "shared/session-protocol.md", + "shared/session-db-mandate.md", + "shared/teach-back-protocol.md", ++ "shared/recording-protocol.md", + "shared/network-bridges.md", + "shared/break-science.md", + "shared/wind-down-protocol.md", +diff --git a/scripts/verify/plan_integration.py b/scripts/verify/plan_integration.py +index 7de99fc3..3e756ca1 100644 +--- a/scripts/verify/plan_integration.py ++++ b/scripts/verify/plan_integration.py +@@ -62,8 +62,9 @@ GOLDEN_SHA256 = ( # pragma: allowlist secret + "ec451ce8857c8a72e398e3054e3c060cd3b5b13ecb29e29cba3822dc192503c0" # pragma: allowlist secret + ) + +-#: The production inventory: 23 original tools + the nine plan tools (review 3, F13). +-PRODUCTION_TOOL_COUNT = 32 ++#: The production inventory: 23 original tools + the nine plan tools (review 3, F13) ++#: + record_teachback (learning tier item 1, 2026-09-23). ++PRODUCTION_TOOL_COUNT = 33 + CORE_TOOLS = frozenset({"list_courses", "get_study_backlog", "end_session"}) + + #: Protected test files: byte-identical to their base since the programme began. +@@ -148,7 +149,7 @@ def check_golden_sha(repo_root: Path) -> tuple[int, dict[str, Any]]: + + + def check_inventory_in_process(repo_root: Path) -> tuple[int, dict[str, Any]]: +- """The in-process twin of the stdio inventory: exactly 32 unique names, ++ """The in-process twin of the stdio inventory: exactly 33 unique names, + the nine plan tools, ``record_plan_learning`` and the core names.""" + _ = repo_root + from studyloop.mcp.server import mcp +``` diff --git a/docs/architecture/learning-tier/council/review-9-arbitration-2026-09-23.md b/docs/architecture/learning-tier/council/review-9-arbitration-2026-09-23.md new file mode 100644 index 000000000..eae52ad21 --- /dev/null +++ b/docs/architecture/learning-tier/council/review-9-arbitration-2026-09-23.md @@ -0,0 +1,123 @@ +# Arbitration — council review 9 (issue #38: the mentor writes, learning tier item 1) + +**Reviewed tree:** `feat/learning-tier-fed` @ `3c175f78` (5 commits on `main` `c1a28de1`). +**Brief:** `brief-review9-2026-09-23.md` (sha256 `f2217677…`, 1212 lines: binding context, commits, the +protocol and S1-0 receipt verbatim, the full source/test/definition diff). **Receipts:** `review9/` — one +transcript per seat; the pre-commit whitespace hooks normalise trailing whitespace and final newlines, so +`git diff -w` against the originals is empty and the originals' sha256 prefixes are `b9c0f2bdab8956fa` +(grok-4.6), `cbf7e06937f97dd8` (openai.gpt-6-astra), `1245f34100fa4929` (qwen3-coder). +**Corrections landed on:** `ad6d1f9d`, `019624c3`, `2a4d4d1d`, `616fd1ed` (tree `616fd1ed`). + +## Seats and verdicts + +| Seat | Verdict | 🔴 | 🟡 | +|---|---|---|---| +| openai.gpt-6-astra | ACCEPT-WITH-CORRECTIONS | 0 | 5 | +| grok-4.6 | ACCEPT | 0 (two "🔴-shaped" S1-SIM gates, not merge gates) | 5 | +| qwen3-coder | REJECT | 2 | 1 | + +### Method + +Every 🔴/🟡 was checked against the tree before anything was changed — by running the code (`coerce_scores` +on bools and floats), reading the function bodies the finding cites (`record_teachback`'s `except` clauses, +`records.bind`/`_owner`, `observations.record`), or grepping the file the finding names. A finding whose +claim the tree contradicts is refuted below with the evidence; one that holds is landed one coherent commit +per concern (findings that share a file share a commit and are both named in its message). Deviation from +"one commit per finding" stated here on purpose. + +### Findings and dispositions + +- **grok Y5 — blank `concept`/`topic` accepted.** Verified: the columns are `NOT NULL` (`migrations.py:454, + 506`) and `""` satisfies `NOT NULL`. **Landed `019624c3`:** refused at the tool boundary before any write; + three parametrised tests. The CLI has the same hole (a click argument can be `""`); left, as the finding + says, "not the defect this branch is for". +- **grok Y1 — one `ToolError` for three failures.** Partially verified: `record_teachback` returns `False` + for no connection, `sqlite3.IntegrityError` and `sqlite3.OperationalError` (lines 93, 201, 214) and does not + say which. **Landed `019624c3`:** the message now states the three causes instead of claiming + "unavailable". Not landed: a real split, which needs the writer to distinguish them — the CLI inherits the + same collapse — and is out of this branch's scope. The finding's other clause ("swallows `ScopeError`") is + **false**: `ScopeError` is not caught; it propagates. +- **grok Y2 — Grok's projection is asserted, not pinned.** Verified true as stated. **Landed `ad6d1f9d`:** + `test_grok_projects_the_codex_definition` pins the adapter's documented projection and that `agents/grok/` + does not exist (so a split fails loudly). Checking this finding's mechanism produced the coordinator's own + finding below. +- **grok Y3 — the `consent` column is documentation, not a runtime check.** True by design and the seat says + so; accepted as recorded. **Not code:** it becomes S1-SIM's refusal-path script (item 2 of the seat's list), + which is the claim-blocker for "pipe open", not a merge blocker. +- **astra Y2 — the no-new-grant guards are weaker than their names.** Verified: the Kiro guard tested an exact + intersection only (a `@studyloop/*` wildcard would pass it); the OpenCode guard was four substring checks + (an added allow rule would pass). **Landed `ad6d1f9d`:** the Kiro guard also refuses any wildcard entry; the + OpenCode block is parsed from the frontmatter and compared whole. +- **astra Y3 — "projected byte-identically" is not what the tests establish.** Verified: the receipt's §2 + wording overstated the mechanism; the implementation is one file, hashed, plus an identical reference + sentence and the names in each definition. **Landed `ad6d1f9d`:** receipt §5(b) states the actual + mechanism and marks installed-path resolution for Codex/Claude/OpenCode **UNVERIFIED** (a pre-existing + question for all nine shared protocols) → S1-SIM. The seat's proposed six-adapter resolution test is not + built here; it is the SIM stage. +- **astra Y4 — "each writer's row landed" exceeded the assertions.** Verified: the test stat'ed files. + **Landed `616fd1ed`:** rows counted in the sandbox database (`teach_back_scores`, `parked_topics`), the + topics file must carry the concept. +- **astra Y5 — no explicit unsuccessful-write path in the protocol.** Verified. **Landed `2a4d4d1d`:** a + paragraph — say it in one line, continue, never retry silently, never claim a record that did not land. +- **qwen R2 — the `when:` strings are vague.** Partially accepted, not as a blocker: the trigger's + observable part (five scores proposed in one sentence; the learner's next reply accepts them) is now in + the `when:` text (`2a4d4d1d`). The consent column stays vocabulary, as grok Y3 argues. + +### Coordinator's own finding (found while verifying grok Y2) — landed `ad6d1f9d` + +`studyloop study` does not hand any adapter the installed definition. It renders +`agents/shared/personas/.md` through `agent_launcher.build_canonical_persona` and gives that string +to every adapter's `setup()` (`agent_launcher.py:76, 273–`; `adapters/_strategies.py`). So the six files +GREEN named the writers in are what a learner gets when opening a harness directly, and the **live** study +persona (`study.md`) named no writer — it told the mentor to *show the learner* a `studyloop topic` command +and never to write anything itself. That is the defect in its exact live form, and none of the three seats +saw it (grok's table says "Codex / pi / Grok — shared See-line on `AGENTS.md`", which is the installed +file). Fixed: `study.md` gains a Recording section and lists the writers; `co-study.md` names the same four +under its student-drives rule; `test_the_built_live_persona_names_each_writer` pins the **built** string for +both modes; the docs contract adds both live personas to `DEFINITIONS`; receipt §5(a) records the +correction. The RED as first written was **vacuous on this axis** — it pinned files no live session reads — +which is the kind of gap question (d) of the brief asked about, and the seats did not find it. + +### Rejected or not taken, with reasons + +- **grok Y4 — thread `study_session_id` into `observations.record(source_session_id=…)`.** The seat's + mechanism is wrong. `observations.record` (`history/observations.py:66–74`) treats `source_session_id` as + a *native* session with captured evidence: it calls `capture_session_input(conn, source_session_id)` and + raises `ScopeError("Source-linked progress requires nonempty captured input")` when there is none. A study + session id has no captured input, so the write would fail exactly as the RED's attempt did through + `records.bind`. Rejected; N1's report-only resolution stands (astra: "Accept the correction for this + patch"). Session-level provenance for a teach-back is a storage-layer question for its own issue. +- **qwen R1 — "the test still asserts the old expectation".** False. `test_mcp_teachback.py:166–184` + asserts `result["study_session_id"] == "study-7"` and the row's `session_id` is `None`, which is the + corrected expectation. The seat appears to have read the RED commit's text rather than the tree. +- **qwen Y — `tutor-checkpoint` residue in the Kiro persona.** False. `grep tutor-checkpoint + agents/kiro/study-mentor/persona.md` returns nothing on `3c175f78`. The CLI examples the seat names + (`studyloop teachback`, `session-query`) are the learner's own path, kept on purpose. +- **qwen's REJECT** rests on R1 and R2; R1 is refuted and R2 is a wording improvement. The verdict is not + sustained. +- **astra Y1 (UNVERIFIED by the seat) — coercion may accept bools/floats.** Refuted by running it: + `coerce_scores([True]*5)`, `[3.0,3,4,3,2]` and `[3.7,3,4,3,2]` all raise `ValueError("scores must be + integers from 1 to 4")` — the `str()` round-trip makes `int("True")` and `int("3.0")` fail. Digit strings + are accepted, which mirrors the CLI's shell input by design. + +### Verification after fixes (tree `616fd1ed`) + +RED files + install contracts + isolation + CLI + launcher + prompt contract + session-start-purpose + +plan-architect persona: **257 passed**. ruff, format, pyright clean on every touched file; mkdocs strict +clean; `agents/manifest.json` regenerated (only `shared/recording-protocol.md` changed hash); +`.secrets.baseline` via whole-repo scan (72 → 72 files, no entry moved). + +### Process findings + +- A second seat probe with `max_tokens=4` returned "empty content" for the two reasoning models — a budget + artefact, not an auth failure; the real run (16 000) answered in 50 s / 42 s / 7 s. +- Verifying a seat's *mechanism* (grok Y2, grok Y4) rather than only its *claim* is what surfaced the live- + persona gap and refuted the `source_session_id` proposal. Both would have passed a claim-level check. + +## Gate decision + +**ACCEPT** for merge at `616fd1ed`, subject to CI. The S1-SIM stage remains owner-gated and is the only +place the claim *"pipe open on {passed}; plumbing proven for all six definitions; noticing observed once"* can +be made; grok's ten-item SIM list (seat transcript §(e)) is adopted as that stage's checklist, with the +refusal path (item 2), Claude `tools:`-line reachability (item 7) and installed-path resolution of +`agents/shared/…` (astra Y3) as the three that decide whether a harness is in `{passed}`. diff --git a/docs/architecture/learning-tier/council/review9/manifest.json b/docs/architecture/learning-tier/council/review9/manifest.json new file mode 100644 index 000000000..c97336ef4 --- /dev/null +++ b/docs/architecture/learning-tier/council/review9/manifest.json @@ -0,0 +1,47 @@ +{ + "run_at": "2026-09-23T10:33:23+00:00", + "brief": "docs/architecture/learning-tier/council/brief-review9-2026-09-23.md", + "brief_sha256": "f221767716ca755a2d3e596eaeacb05d9596688dfef9e26162c378fe903de7ec", + "system_sha256": "5f21e273f399cdb0455a9fadb8575cc3e231ce0d7931f517206335c98c3f934b", + "seats": [ + { + "model": "openai.gpt-6-astra", + "ok": true, + "reasoning_chars": 0, + "finish_reason": "stop", + "elapsed_s": 50.3, + "usage": { + "prompt_tokens": 17243, + "completion_tokens": 2716, + "total_tokens": 19959 + }, + "error": null + }, + { + "model": "grok-4.6", + "ok": true, + "reasoning_chars": 0, + "finish_reason": "stop", + "elapsed_s": 41.6, + "usage": { + "prompt_tokens": 18591, + "completion_tokens": 4728, + "total_tokens": 23319 + }, + "error": null + }, + { + "model": "qwen3-coder", + "ok": true, + "reasoning_chars": 0, + "finish_reason": "stop", + "elapsed_s": 7.2, + "usage": { + "prompt_tokens": 17654, + "completion_tokens": 462, + "total_tokens": 18116 + }, + "error": null + } + ] +} diff --git a/docs/architecture/learning-tier/council/review9/seat-grok-4.6.md b/docs/architecture/learning-tier/council/review9/seat-grok-4.6.md new file mode 100644 index 000000000..638acc05b --- /dev/null +++ b/docs/architecture/learning-tier/council/review9/seat-grok-4.6.md @@ -0,0 +1,132 @@ +I'll inspect the tree and the cited tests before judging N1, the trigger table, and decision 2. I'll start from the commits and the files the brief names.I'll pull the GREEN tool, the ownership bind, and the RED/GREEN tests next so N1 is judged from the code, not the brief.The bind path and the four inventory pins are next — those decide whether N1 is the right fix or a workaround.I have enough of the storage layer. Next I'll check whether the protocol is actually projected, whether any RED is vacuous, and what S1-SIM still has to prove.The protocol is referenced, not inlined. I'll confirm the remaining harness files and the TDD commits before writing findings.I have the tree. Next I'll pin a few remaining claims (N1 alternatives, inventory, TDD order) and then write the findings.I have the facts. One more pass on N1 alternatives and the trigger-table consent column, then the verdict. Let me also check a few more details that might be findings.I have enough. Checking two last details that could flip a finding: whether `observation.source_session_id` is a real alternative, and whether the Kiro JSON `tools` surface still exposes writers without naming them.Last check: whether the RED commit actually failed for the stated reasons, and whether Claude's `tools:` line is instruction-only as claimed.I have the evidence. Writing the review.The RED/GREEN split and N1 correction are documented. Checking one last possible hole: whether `record_teachback` actually upserts `study_progress` (so tomorrow's recommendation can move) and whether the protocol's consent column is enforced anywhere but prose.I have the tree, the tests, and the bind layer. Writing the review.# Review 9 — `feat/learning-tier-fed` @ `3c175f78` + +## Findings + +### 🔴 must fix before merge + +None that block the pipe-open claim on the code that is here. The two 🔴-shaped holes are S1-SIM gates, not merge gates for this tree — listed under (e). + +### 🟡 should fix + +- 🟡 `packages/studyloop/src/studyloop/mcp/tools.py:910-918` — `record_teachback` swallows `records.bind` `ScopeError` / `IntegrityError` into the same `ToolError("…database is unavailable")` the no-connection path uses. A scope-misconfigured write and a missing DB are different failures; the CLI's `False` collapse is inherited, but the MCP surface is the one a stranger's mentor will see. Split the message (or let `ScopeError` through) so S1-SIM can tell "DB down" from "scope rejected the bind". +- 🟡 `packages/studyloop/tests/test_docs_harness_tier_contract.py:180-186` + `test_adapter_parity.py:111-118` — `TestRecordingProtocol.DEFINITIONS` lists five files and omits Grok; `MENTOR_DEFINITIONS` maps `"grok"` onto `agents/codex/AGENTS.md`. That is correct *today* (`adapters/grok.py` projects the Codex file) but the contract test does not pin the adapter projection. A later `agents/grok/` split would keep the five-file test green while the sixth definition silently dropped the protocol. Add an assertion that `adapters/grok.py` still writes `AGENTS.md` from the Codex source (or include Grok in `DEFINITIONS` via the adapter). +- 🟡 `agents/shared/recording-protocol.md` trigger table, `consent:` column — the four values (`learner_agreed` / `learner_stated` / `learner_confirmed`) are documentation, not a tool argument and not a test. That is the right signal (the learner's words, never a transcript inference) **and** it is unenforced. The tool will happily record scores the learner never agreed to if the model calls it. Acceptable under decision 2 (prompt-per-call, no new grants) only if S1-SIM scripts a refused-consent turn on every harness and asserts **zero** rows. Not a merge blocker; it is the claim-blocker for "pipe open". +- 🟡 `packages/studyloop/src/studyloop/mcp/tools.py:910` + `history/teachback.py:155-157` — `study_session_id` is read from session state and returned, then discarded. `record_teachback` still upserts `study_progress` (`history/teachback.py:196-204`), so tomorrow's recommendation *can* move — the MVP defect is addressed at the card level. What is *not* linked is "this teach-back happened in study session X". The observation row `record_teachback` writes (`history/teachback.py:187-194`) currently carries no `source_session_id`. If session-level provenance matters for the noticing episode (decision 3), thread `study_session_id` into `observations.record(..., source_session_id=...)` — that column is the storage layer's supported study-session link, and it does not go through `records.bind`. See N1 below. +- 🟡 `packages/studyloop/tests/test_mcp_teachback.py:102-123` — validation cases miss empty `concept` / empty `topic` (both `NOT NULL` in the schema, both accepted as `""` by the tool). CLI has the same hole. Not the defect this item exists to fix; one parametrized case would stop a silent empty-concept row from counting as a fed tier. + +### 🔵 style + +- 🔵 `packages/studyloop/src/studyloop/mcp/tools.py:900-902` — three imports inside the tool body. Matches neighbouring writers; fine. If anything moves, keep `coerce_scores` / `coerce_review_type` imported the same way the CLI now does (from `history.teachback`), which this already does. +- 🔵 `packages/studyloop/tests/test_mcp_plan_tools.py:937` — test renamed `thirty_two` → `thirty_three` and the three sibling pins (`test_mcp_stdio_smoke.py:48`, `scripts/verify/plan_integration.py:67`, `test_verify_plan_integration_script.py:467`) moved in the same commit (`3c175f78`). Inventory pin discipline is correct. +- 🔵 `agents/kiro/study-mentor/persona.md:32` — the old `uv run tutor-checkpoint` line is gone; the CLI block now shows `studyloop teachback` as the human path. Good. Residual `tutor-checkpoint` still appears in OpenCode's `permission:` bash allow-list (`agents/opencode/study-mentor.md`, S1-0 receipt §2) — that is the recorded wildcard, not a routing instruction. Leave it. + +### 💡 learning + +- 💡 Finding N1 is the interesting one. The ownership layer (`agent_session_tools.context.records.bind`) treats `session_id` as a *native harness session* that must exist in `sessions` and be visible in scope. Study sessions are a different id space; the only tables allowed to carry `study_session_id` are `parked_topics` and `study_notes`. Stuffing a study id into `teach_back_scores.session_id` cannot work without a schema/ownership change that this item is not licensed to make. Reporting-without-storing is the correct *storage* resolution. The missed opportunity is the observation: `observations.record` already has `source_session_id` and does not go through `bind`. GREEN should have passed the live study id there. That is a one-line addition plus one assert in `test_the_study_session_is_reported_not_stored_as_the_rows_session_id`. It does not change the row's `session_id` (still `NULL`, still owned by scope, still matches the CLI). Do it on this branch if the noticing episode needs "which session did this teach-back belong to"; otherwise file it as the first S1-SIM observation and do not block merge. +- 💡 `test_writer_isolation.py` passed on first run and is honestly labelled a guard. That is the right TDD call. The child-process `HOME`/`XDG_*` decoy is the test that would have caught the 2026-09-05 home-directory leak; keep it. + +--- + +## Judgements the brief asked for + +### (a) Trigger table complete? Consent column the right signal? + +Complete for `W_auto`. Four triggers, four writers, bijection pinned by `test_every_trigger_names_a_w_auto_writer_and_every_writer_has_a_trigger`. No SRS mutator is named (`test_no_trigger_names_an_srs_mutator`). The `when:` lines match the standing rule (learner's agreement is the signal; low-energy blank is never scored). + +Consent column is the right *vocabulary* (`learner_agreed` / `learner_stated` / `learner_confirmed` — never `model_inferred`). It is not a runtime check. That is acceptable as instruction-parity (decision 2) and insufficient as a guarantee. S1-SIM must script the refusal path. + +One gap in the table itself, not a 🔴: `required_ids` for `teach_back_agreed` is `[concept, topic, scores, review_type]` and does not mention that `session_id` is bound from state, not the caller. The tool docstring says this; the table does not. Fine — the table is for the model, the docstring is for the caller. + +### (b) Decision 2 honoured? + +Yes, on every harness this repo can express. + +| Harness | Names | Grants | +|---|---|---| +| Kiro | persona.md:32 and :79-81 name all four; JSON `allowedTools` still only `@studyloop/log_topic` | `test_kiro_gains_no_pre_approval` pins the set intersection | +| Claude | `tools:` line names the four `mcp__studyloop__*` writers plus the five readers | `test_claude_settings_grant_nothing` — no `permissions` block | +| OpenCode | protocol See-line names them | `test_opencode_permission_block_is_exactly_as_recorded` — wildcard unchanged | +| Codex / pi / Grok | shared See-line on `AGENTS.md` (Grok via projection) | no grant grammar in repo | + +Claude's `tools:` addition is naming, not approval — the S1-0 receipt is explicit that an unnamed MCP tool is unreachable on that sub-agent, so naming is what makes the pipe exist at all. That is instruction, and it is required. No new pre-approval. + +Kiro's existing `log_topic` pre-approval is untouched. The one recorded asymmetry stays one. + +### (c) N1 — reporting-without-storing, or bind another way? + +**Reporting-without-storing is the right resolution for `teach_back_scores.session_id`.** + +Evidence: + +- `records.bind(..., session_id=...)` requires a row in `sessions` visible in scope; otherwise `_owner()` raises `ScopeError("Source session is unavailable in the configured scope")`. +- Study-session links are permitted only for `parked_topics` and `study_notes`. +- The CLI already writes `session_id=NULL` and owns the row by scope. MCP matching the CLI is the S1-0 contract ("land through the same function"). +- The RED as first written (`f439ab84` docstring still on the GREEN test) assumed the live study id would land in the column. GREEN recorded the correction in the test name and docstring (`test_the_study_session_is_reported_not_stored_as_the_rows_session_id`). That is the TDD rule honoured: a RED expectation corrected at GREEN is recorded, not erased. + +**The alternative that the storage layer *does* support**, and that GREEN should have taken as a sidecar, not a replacement: + +``` +observations.record(..., source_session_id=study_session_id) +``` + +`history/teachback.py:187-194` already writes an observation on every successful teach-back. `source_session_id` is the column the ownership layer uses for study-session provenance without going through `bind`. Passing the live id there: + +- does not change `teach_back_scores.session_id` (stays `NULL`, stays legal); +- does not require a schema change; +- gives the noticing episode (decision 3) a join from teach-back → study session; +- is what the brief's "e.g. the observation's `source_session_id`" is pointing at. + +Do **not** invent a `teach_back_scores.study_session_id` column on this item. Do **not** pass the study id as `session_id` to `record_teachback` — that is the failure the RED discovered. Report in the reply (done) + optionally stamp the observation (not done, 🟡 above). The test as written is the correct assertion for the column it checks. + +### (d) Vacuous REDs / over-claiming GREENs? + +TDD order is clean: + +- `f439ab84` S1-RED — 22 failed, 0 passed. Failures are the ones the plan named: tool absent, writers unnamed, protocol absent, `tutor-checkpoint` still on the Kiro persona, inventory still 32. +- `7354ea3a` S1-GREEN (tool) — N1 correction recorded in the test, not erased. +- `0a22aabe` S1-GREEN (definitions) — protocol projected, persona repointed. +- `3c175f78` inventory 32 → 33 in all four pins. + +Not vacuous: + +- `test_the_caller_cannot_supply_a_session_id` — inspects the signature. Real constraint. +- `test_a_valid_call_lands_exactly_one_row_through_the_real_schema` — hits the migrated CHECK, not a hand-rolled table. +- `test_a_repeated_call_is_two_rows_because_teachbacks_are_events` — pins the "events, not state" rule. +- `test_no_connection_is_a_tool_error_not_a_silent_success` — the silent-success path is the failure mode the receipt forbids. +- `test_kiro_gains_no_pre_approval` / `test_claude_settings_grant_nothing` / `test_opencode_permission_block_is_exactly_as_recorded` — these are the decision-2 pins. They fail if anyone "helpfully" pre-approves. + +Slightly soft, not vacuous: + +- `test_every_mentor_definition_names_each_w_auto_writer` is a `\bword\b` search. A mention in a "do not call these" sentence would pass. The companion `test_claude_mentor_tools_line_names_each_writer` is the one that actually makes Claude able to call them. Acceptable: the protocol See-line is the named instruction, and the word-search is the cheap pin that the See-line exists on every file. +- `test_writer_isolation.py` is a guard and says so. It does not over-claim RED. + +GREEN does not over-claim "pipe open on six harnesses". The commits claim: tool exists, validates as CLI, lands a row, names are on every definition, no new grants, inventory is 33. That is what the tests prove. The stranger-in-hands claim is S1-SIM's job. + +### (e) What S1-SIM must add before "pipe open on {passed}; plumbing proven for all six definitions" + +The unit suite proves the tool and the instruction. It does not prove a mentor on a harness will fire the trigger. S1-SIM (scripted learner, owner-gated, six harnesses) must add, per harness that passes: + +1. **Happy path, `teach_back_agreed`.** Script: mentor proposes five scores → learner says "yes" → exactly one `record_teachback` call → one row in `teach_back_scores` with those scores → reply contains `recorded: true` and a `study_session_id` that matches the live session (or `null` if the harness has no session state) → mentor's next line matches the "Recorded: …" shape. This is the defect. If this fails on a harness, that harness is not in `{passed}`. +2. **Refusal path.** Script: mentor proposes scores → learner says "no" / low-energy blank → **zero** `record_teachback` calls, **zero** new rows. This is the consent column made measurable. A harness that records on refusal is not in `{passed}`. +3. **The other three triggers, one each.** `stuck_two_rounds` → `log_struggle`; `session_end_concepts` → `log_topic` (one call per concept, status the learner confirmed); `plan_wind_down` → `record_plan_learning` only when a plan is active and the learner agrees. Absence of a plan must produce zero `record_plan_learning` calls. +4. **No-trigger silence.** A mid-session turn that matches nothing writes nothing. Pin the row counts before/after. +5. **Duplicate replay.** Two agreed teach-backs on the same concept → two rows. Events, not upserts. +6. **Decision-2 live check, not just file pins.** On Kiro, `record_teachback` / `log_struggle` / `record_plan_learning` must prompt (only `log_topic` is pre-approved). On Claude, the four writers must be *callable* (named on `tools:`) and must prompt (no `permissions` block). On OpenCode, the MCP call prompts at harness default; the bash wildcard is unchanged and must not be the path the mentor takes — if the scripted session shells out to `studyloop teachback` instead of calling the MCP tool, that harness fails the protocol even if a row lands. Codex / pi / Grok: harness-side approval, record whatever the harness does, do not change it. +7. **Claude reachability.** Before this tree the sub-agent could not see any `mcp__studyloop__*` tool. S1-SIM on Claude is the first time that `tools:` line is proven live. If Claude's runtime ignores a `tools:` line that long, the unit test is green and the pipe is closed. This is the highest-risk harness. +8. **Grok projection.** Confirm the session dir's `AGENTS.md` is the Codex file *and* that the running Grok mentor names the four writers. The unit suite never launches Grok. +9. **N1 live.** After a successful write, `teach_back_scores.session_id IS NULL` and the tool reply's `study_session_id` equals the live study session (or `null`). If anyone "fixes" N1 by stuffing the study id into `session_id`, the write will `ScopeError` and the tool will say "not recorded" — that is a SIM-fail, and it is how we find out someone re-broke it. +10. **Isolation under a real harness, not just the child-process guard.** One SIM run with `HOME` pointed at a decoy. If a harness adapter ignores `STUDYLOOP_*` and writes through `~`, that is the 2026-09-05 bug back. + +Do **not** claim "plumbing proven for all six definitions" until (1), (2), (6) and (7) have an owner-gated pass on each of the six. (3), (4), (5), (8), (9), (10) can be a second SIM pass; they are not the MVP defect, but they are what stops the next silent failure. + +UNVERIFIED (not run on this seat): the 7340-pass figure, the four inventory pins going green at `3c175f78`, and `test_concatenated_remote_dump_with_existing_archive` still failing identically to `main`. Coordinator to confirm against the tree. + +--- + +## Verdict + +**ACCEPT** + +The MVP blocker is closed in code: there is an MCP `record_teachback`, it shares the CLI validator, it lands a row the same way the CLI does, every harness definition names the four writers and points at one protocol, and no harness gained a pre-approval. TDD order holds. Decision 2 holds. N1 is the correct storage resolution; the observation-`source_session_id` sidecar is a should-fix, not a must-fix. + +Do not merge-gate on S1-SIM — decision 3 already scheduled the noticing episode as observation, not gate — but do not let anyone write "pipe open on six" until the SIM list above has owner ticks. The 🟡s can land as follow-up commits on this branch or the first 0.5.x patch; none of them re-closes the writer. diff --git a/docs/architecture/learning-tier/council/review9/seat-openai.gpt-6-astra.md b/docs/architecture/learning-tier/council/review9/seat-openai.gpt-6-astra.md new file mode 100644 index 000000000..358baa610 --- /dev/null +++ b/docs/architecture/learning-tier/council/review9/seat-openai.gpt-6-astra.md @@ -0,0 +1,79 @@ +## Findings + +Reviewed the supplied source and diffs; no repository execution performed. Runtime conclusions below are marked **UNVERIFIED**. + +- 🟡 **UNVERIFIED — `packages/studyloop/src/studyloop/mcp/tools.py:868–873`; `packages/studyloop/tests/test_mcp_teachback.py:47–51` — MCP input coercion may defeat the claimed CLI-equivalent integer validation.** + Every new functional test calls the registered `.fn`, bypassing FastMCP argument validation. With `scores: list[int]`, transport validation may convert booleans or integral floats before `coerce_scores()` can reject them; the CLI rejects `"True"` and `"3.0"`. + **Correction:** add `test_wire_validation_matches_cli` through the actual MCP invocation path, covering booleans, integral/fractional floats, numeric strings, malformed strings, wrong lengths and range violations. Assert rejection leaves storage unchanged. If coercion changes acceptance, make the boundary strict or preserve raw values for the shared validator. **Done:** documented acceptance agrees across both surfaces, including transport validation. + +- 🟡 **`packages/studyloop/tests/test_adapter_parity.py:153–176` — the “no new pre-approval” guards do not establish what their names claim.** + Kiro’s exact-name intersection would miss a newly granted server wildcard; OpenCode’s substring checks permit additional allow rules while all assertions remain green. The supplied implementation does not introduce those grants, but the regression contract is incomplete. + **Correction:** compare the complete Kiro `allowedTools` and parsed OpenCode permission structure with the recorded baseline. Add mutation tests showing that an additional wildcard or writer-specific approval fails. Retain Claude’s prohibition on a `permissions` block. + +- 🟡 **`packages/studyloop/tests/test_docs_harness_tier_contract.py:228–231`; `agents/codex/AGENTS.md:15` — reference presence is being treated as protocol projection.** + The definitions add links; the test establishes only that the filename appears. This does not establish the receipt’s stronger assertion that the protocol is “projected byte-identically into every definition,” nor that an installed mentor can resolve it. Links are a valid implementation **if distribution and resolution work**; inlining is not inherently required. + **Correction:** describe the actual reference-based mechanism in a GREEN addendum to the receipt. Add an installation/projection test, such as `test_recording_protocol_resolves_for_each_installed_harness`, which exercises all six adapters and compares the resolved file’s bytes with the canonical protocol. **UNVERIFIED:** actual installed-path resolution. + +- 🟡 **`packages/studyloop/tests/test_writer_isolation.py:110–119` — “each writer’s row landed” exceeds the assertions.** + A shared database file and successful-looking replies do not prove that both `record_teachback` and `log_struggle` persisted their intended rows. Likewise, a session-topics file does not prove the expected concept was recorded. + **Correction:** query the sandbox database for exactly the expected teach-back and parked-topic records, and assert the session file contains the expected topic/status. Retain the decoy-home check. This remains an isolation **guard**, not retroactively a RED. + +- 🟡 **`packages/studyloop/src/studyloop/mcp/tools.py:917–921`; `agents/shared/recording-protocol.md:54–56` — failure reporting needs an explicit unsuccessful-write path.** + The writer’s verified `False` contract includes `sqlite3.IntegrityError`, so “the sessions database is unavailable” is not always a truthful diagnosis. The protocol also gives only a success acknowledgement, with no rule for denied or failed calls. + **Correction:** use a generic “teach-back not recorded” error unless the underlying cause is distinguishable. In the protocol, require “Recorded” only after a successful result; denied/error results must say nothing was recorded. Do not automatically replay an ambiguously completed event write. Add a failure-path test and denial/error scenarios to S1-SIM. + +## Required judgments + +### Trigger coverage and consent + +**Complete for the four specified writers, not yet behaviorally proven.** + +- Teach-back scores require explicit agreement; the low-energy exception is preserved. +- Topic status requires confirmation. +- Plan-learning wording requires agreement. +- Struggle logging uses a learner-stated difficulty, not permission inferred from harness approval. + +The struggle YAML `when` is weaker than its prose: “without a breakthrough” could describe the mentor’s inference. Align it with the prose: **the learner has stated continued difficulty across two rounds on the same point**. Require `when` in `TestRecordingProtocol.REQUIRED_KEYS` and test the intended consent values, not merely column existence. + +Learner consent and harness tool approval are separate signals. A pre-approved Kiro `log_topic` call still requires confirmed status; approving an MCP invocation does not itself establish agreement with proposed scores. + +### Decision 2 + +**Honoured in the supplied diff.** All six definitions name `W_auto`, with Grok using Codex’s canonical definition. Claude’s frontmatter makes the writers reachable without adding permissions. Kiro and OpenCode approval configuration is unchanged. No trigger selects `W_srs`. + +The instruction “one prompt per call, on every harness” for `W_srs` cannot itself enforce external harness policy. Read it as intended behavior, not a demonstrated approval guarantee. + +### N1 + +**Accept the correction for this patch.** + +Passing a study-session ID as a native `sessions` owner is wrong. Scope-owned rows, matching the CLI, are a defensible resolution; returning `study_session_id` provides reply context, **not durable session provenance**. + +**UNVERIFIED:** whether an observation’s `source_session_id` accepts study-session IDs. Its name is not evidence of compatibility. Do not move the same incompatible ID there speculatively. A future durable association needs a verified observation API/schema contract or an explicit supported study-session relationship, plus scope tests. + +The supplied test documents the changed expectation. **UNVERIFIED:** the GREEN commit’s historical annotation, beyond the coordinator’s stated commit account. + +### RED/GREEN claims + +The positive persistence and explicit rejection tests are meaningful, not vacuous. The N1 expectation change is justified rather than a concealed relaxation. Inventory changes correctly retain exact counts. + +The weak guards identified above over-claim coverage. **UNVERIFIED:** committed failing execution of every stated RED; no such execution was performed here. + +No-trigger behavior is not exercised by the supplied additions. A test that merely refrains from calling a writer would be vacuous; this needs a mentor/harness execution. + +## S1-SIM completion criteria + +Before making the proposed plumbing claim, retain per-harness evidence for: + +1. **Installed configuration:** canonical protocol resolves, all four writers are discoverable, and approval configuration matches the frozen baseline. +2. **Positive triggers:** a scripted learner causes each writer to persist the expected record—without manually selecting the writer on the mentor’s behalf. +3. **Negative triggers:** unagreed scores, a low-energy blank, unconfirmed status, declined plan text, and unrelated conversation produce no corresponding writes. +4. **Failure and replay:** denial/error causes no success acknowledgement; two deliberately submitted teach-back events produce two rows. +5. **Tomorrow’s read:** restart the session/server and show the teach-back is visible to the learning reader and reaches the recommendation’s input. The top recommendation need not necessarily change. +6. **Receipt:** list harness versions, approvals, tool calls, storage evidence and explicit pass/fail/unrun outcomes. Name only passed harnesses as “pipe open”; static definition checks do not prove six live pipes. + +Keep S1-SIM owner-gated. Keep the first-real-session noticing episode scheduled after merge and observational, not an added merge gate. + +## Verdict + +**ACCEPT-WITH-CORRECTIONS** — the writer path and N1 resolution are sound on the supplied evidence; validation-boundary coverage, regression guarantees and evidence claims need tightening. diff --git a/docs/architecture/learning-tier/council/review9/seat-qwen3-coder.md b/docs/architecture/learning-tier/council/review9/seat-qwen3-coder.md new file mode 100644 index 000000000..54126418a --- /dev/null +++ b/docs/architecture/learning-tier/council/review9/seat-qwen3-coder.md @@ -0,0 +1,21 @@ +🔴 `packages/studyloop/tests/test_mcp_teachback.py:122` — The RED expectation that `session_id` would be stored was corrected in the GREEN to report-only (finding N1); the test should align: it still asserts the old (incorrect) expectation, making the RED vacuous. + +🔴 `agents/shared/recording-protocol.md:15` — The trigger table uses `when:` strings containing vague terms like "the learner agreed them" and "the learner confirmed"; criteria must be observable and specific (e.g., "after proposing 5 scores in one sentence, and receiving textual affirmation") to ensure reproducible harness behavior. + +🟡 `agents/kiro/study-mentor/persona.md:105` — File appends protocol ref without removing outdated `tutor-checkpoint` instructions fully—residual indirect references remain in CLI examples (`studyloop teachback`, `session-query`) that contradict §4 instruction (“never parity of approval”). + +🔵 `packages/studyloop/tests/test_mcp_teachback.py:9`–`215` — Newly introduced MCP test file duplicates extensive coverage already present in `test_writer_isolation.py:9`–`119` with respect to writer invocation and environment isolation; consolidation possible to reduce redundancy. + +💡 — UNVERIFIED: Whether scripted simulations across all six harnesses (S1-SIM stage) pass remains unconfirmed; a definitive claim about “plumbing proven for all six definitions” is premature pending integration verification. + +Verdict: **REJECT** + +Criteria not met: +- (a) The trigger table’s `when:` clauses lack precision for reproducibility. +- (c) Finding N1’s resolution conflicts with current test assertion; RED/GREEN integrity broken. +- (d) One RED (`test_the_study_session_is_reported_not_stored_as_the_rows_session_id`) misaligns with updated behavior, thus becomes vacuous. + +Fix Required Before Merge: +1. Correct `test_mcp_teachback.py` to match actual GREEN behavior (report-only session ID). +2. Refine trigger conditions in `recording-protocol.md` to specify observably confirmable actions. +3. Remove residual misleading CLI command refs in Kiro persona doc. diff --git a/docs/architecture/learning-tier/plan-2026-09-19.md b/docs/architecture/learning-tier/plan-2026-09-19.md index 6818505b0..4e17fad2d 100644 --- a/docs/architecture/learning-tier/plan-2026-09-19.md +++ b/docs/architecture/learning-tier/plan-2026-09-19.md @@ -9,9 +9,10 @@ outstanding:** §7. **Base:** `main` @ `4f8e3e0f`. **Item 1 — the mentor writes.** Today the mentor agent can *read* learning state on every harness but records nothing: no MCP teach-back writer exists (CLI only), kiro pre-approves one writer, claude grants none, the persona's only concrete "record progress" line points at a different tool. Result: zero writer calls in -143,973 historical messages. This item adds the missing writer, grants the additive writers on the harnesses -that have an in-repo grant grammar, gives every harness one machine-readable trigger table, and proves the -pipe is open with a scripted learner on all six installed harnesses — claiming only what was observed. +143,973 historical messages. This item adds the missing writer, names the additive writers in every harness +definition (no new pre-approvals — owner decision 2, §7), gives every harness one machine-readable trigger +table, and proves the pipe is open with a scripted learner on all six installed harnesses — claiming only what +was observed. **Item 2 — one honest number for Jev.** A single pre-registered, directional measurement of TypeSafe's Jev as a *filter* over harness-proposed struggle candidates, on the repo's 13-session human-labelled gold read @@ -25,7 +26,9 @@ Jev call before the pre-registration commit. | 1 | `feat/learning-tier-fed` | `~/code/personal/tools/studyloop-wt/tier` | own PR, mergeable independently | | 2 | `feat/jev-struggle-eval` | `~/code/personal/tools/studyloop-wt/jev-eval` | own PR (eval code + receipts only) | -Both cut from `main` @ `4f8e3e0f`. No code dependency either way (item 2 scores frozen gold). Each stage: +Item 1's branch is cut from `main` when S1-0 starts; item 2's is **not cut until milestone 0.5.x is closed** +(owner decision 1, 2026-09-23 — item 1 is MVP work, item 2 a post-MVP measurement). No code dependency either +way (item 2 scores frozen gold). Each stage: commit in coherent steps with why-bodies; full suite diffed against a clean control worktree (only the RED tests may flip); push only when the owner says so. The `feat/jev-judge` planning branch (Stage 0 receipt, this plan) merges first or is cherry-picked — owner's call (§7). @@ -36,13 +39,20 @@ this plan) merges first or is cherry-picked — owner's call (§7). Record, from source, the grant mechanism per harness and freeze the writer sets: - `W_auto` = `log_topic`, `log_struggle`, `record_teachback` (new), `record_plan_learning` — additive, learner- - agreed or learner-stated. Pre-approved. + agreed or learner-stated. **Named in every harness definition; prompt-per-call everywhere** (owner decision 2, + §7). No new pre-approval on any harness this branch; kiro's existing `log_topic` allowlist entry stays as the + one recorded asymmetry (`execute_bash` is pre-approved there, so removing it buys no parity). - `W_srs` = `record_study_progress`, `log_review_outcome`, `record_topic_progress` — SRS mutators that accept an unverified `card_hash`/topic id (`mcp/tools.py:114,648`). **Stay prompt-per-call** this branch. -- Grant grammar: kiro `allowedTools` (`agents/kiro/study-mentor.json`); claude `settings.json` permissions + - tool names in `socratic-mentor.md`; opencode already `studyloop *` (recorded as not least-privilege, unchanged); +- Grant grammar: kiro `allowedTools` (`agents/kiro/study-mentor.json`, unchanged); claude `settings.json` + permissions (unchanged — no block) + tool names in `socratic-mentor.md` (**must name each `W_auto` tool**: + its `tools:` line lists `Read, Write, Grep, Bash` and no `mcp__studyloop__*` tool today, so naming is what + makes the writers reachable at all — naming is instruction, not approval); opencode already `studyloop *` + (recorded as not least-privilege, unchanged); codex/pi/grok have **no in-repo grant grammar** — canonical persona via session-dir `AGENTS.md`, approval is - harness-side (`adapters/{codex,pi,grok}.py`). No `agents/grok/` is created. + harness-side (`adapters/{codex,pi,grok}.py`). No `agents/grok/` is created. Parity is of the *instruction* + (trigger table + `W_auto` set, pinned by test), never of approval — the receipt records each harness's + approval cell in the capability matrix as it is. Finish: `docs/architecture/learning-tier/receipts/s1-0-capability-lock.md` committed with file:line per harness. @@ -50,7 +60,7 @@ Finish: `docs/architecture/learning-tier/receipts/s1-0-capability-lock.md` commi | Test | File | Fails on `main` because | |---|---|---| | `record_teachback` MCP tool exists, validates exactly as `cli/_teachback.py:16-46` (cardinality, ints, 1–4, `review_type` enum), lands one row honouring CHECK, no row on failure, `session_id` bound from session state | `tests/test_mcp_teachback.py` (new) | tool absent | -| kiro `allowedTools` ⊇ `W_auto`; claude permissions ⊇ `W_auto` and `socratic-mentor.md` names each; opencode wildcard covers `W_auto`; codex/pi/grok canonical persona names each `W_auto` tool | `tests/test_adapter_parity.py` (extend) | grants/names absent | +| every harness definition names each `W_auto` tool (kiro `tools`, claude `socratic-mentor.md` `tools:`, opencode persona, codex/pi/grok canonical persona); **no new pre-approval**: kiro `allowedTools` ∩ `W_auto` == {`log_topic`} exactly, claude `settings.json` still has no permissions block, opencode wildcard unchanged; each harness's approval cell in the capability matrix equals what its definition file says | `tests/test_adapter_parity.py` (extend) | names absent | | `agents/shared/recording-protocol.md` exists; fenced YAML trigger table parses; every trigger names a writer in `W_auto`; every projected copy carries identical bytes; `agents/manifest.json` hashes match; `persona.md` no longer routes "record progress" to `tutor-checkpoint` | `tests/test_docs_harness_tier_contract.py` (extend) | file absent; `:32` still names `tutor-checkpoint` | | Isolation: child process with conflicting `HOME`/`XDG_*` runs each `W_auto` writer → 0 writes outside the sandbox, including Markdown | `tests/test_writer_isolation.py` (new) | passes or fails on `main` — if it passes, keep it as a guard, do not count it as RED | | No-trigger + duplicate: a replayed sequence with a non-triggering turn writes nothing; a repeated `record_teachback` call produces the documented behaviour (two rows — teach-backs are events) | `tests/test_mcp_teachback.py` | tool absent | @@ -60,7 +70,9 @@ Finish: N RED committed; `pytest` on a clean control worktree shows **exactly** ### S1-GREEN (1–2 days) - `mcp/tools.py`: `record_teachback` delegating to `history/teachback.py:27` through a **shared validator** extracted from `cli/_teachback.py` (one implementation, both surfaces; CLI kept). -- Grants: kiro `allowedTools` += `W_auto`; claude `settings.json` += `W_auto`, `socratic-mentor.md` names them. +- Names, not grants (owner decision 2, §7): `socratic-mentor.md`'s `tools:` line names each `W_auto` tool and the + readers; kiro `allowedTools` and claude `settings.json` **unchanged**; every other definition names the writers + through the projected protocol. - `agents/shared/recording-protocol.md`: YAML table (`trigger`, `writer`, `required_ids`, `consent`) + one-line prose per trigger: after teach-back **and learner agreement** → `record_teachback`; 2+ rounds without breakthrough → `log_struggle`; end of session → `log_topic` per concept touched; plan wind-down → @@ -164,10 +176,37 @@ between J-b-filtered and J-b-unfiltered*, nothing more. Three-seat council reads 1. **Two branches instead of the one requested** (D13) — accept the split, or insist on one branch and accept that Jev spend/privacy review then gates the agent-definition merge. + - **Owner verdict, 2026-09-23: two branches, and item 2's branch is not cut until milestone 0.5.x is + closed.** Reasoning recorded with the verdict: item 1 is the MVP blocker (the mentor never writes to + the learning tier), item 2 is a post-MVP measurement; and the repository's own history is the argument + against sharing a vehicle — 17 `archive/*` tags whose tips never reached `main`, six parallel branches + opened 7–8 September and archived on the 10th, ~244 commits with no patch-equivalent on `main`. The + owner's self-check ("finished work waiting on unrelated work") was confirmed against that record. + Consequence: decision 4 is deferred to the day S2 starts (see below). 2. **Claude/opencode grants:** approve pre-approving `W_auto` on claude (today: none) and leaving opencode's wildcard untouched this branch. + - **Owner verdict, 2026-09-23: no pre-approval on Claude — prompt per call; OpenCode's wildcard untouched + this branch.** Stated by the owner ("No pre-approval on Claude; prompt per call"), then confirmed by + delegation after the parity question. Evidence that made it cheap: kiro already pre-approved `log_topic` + and opencode already allowed `studyloop *`, and both still logged zero writer calls — the absence is the + *instruction* (no MCP teach-back writer, the persona's record-progress line pointing elsewhere), not the + prompt. Consequences taken with it: no new pre-approval on *any* harness this branch (kiro's `log_topic` + entry stays as the one recorded asymmetry); `socratic-mentor.md` must still *name* each `W_auto` tool, + because its `tools:` line lists no `mcp__studyloop__*` tool today and an unnamed tool is unreachable — + naming is instruction, not approval; parity is pinned on the instruction (trigger table + `W_auto` set), + and each harness's approval cell is recorded in the capability matrix as it is. The S1-0 bullets and the + test-table parity row above were rewritten to this verdict. 3. **The owner-led noticing episode** — ten minutes, once, after S1-GREEN; scheduled when? + - **Scheduled by rule, 2026-09-23 (owner delegated):** the first real study session the owner runs after + S1-GREEN reaches `main` — ten minutes, no logging asked for. The S1-SIM receipt records the date and the + adjudicated trigger/no-trigger outcomes afterwards. The owner may name a date instead; the rule stands + until he does. 4. **Jev spend cap** for S2 (estimate: 13 sessions × ≤ 8 candidates × 2 arms, under $1 at $0.042/Mtok; the candidate-generation harness turns cost credits on the owner's subscription — ~13–26 turns). + - **Deferred, 2026-09-23 (consequence of decision 1):** S2 does not start before 0.5.x closes, so the cap + is decided on the day item 2's branch is cut, not before. Nothing in item 1 depends on it. 5. Merge order for the `feat/jev-judge` planning branch (Stage 0 receipt + this plan): first, or cherry-pick the docs into `feat/learning-tier-fed`. + - **Resolved, 2026-09-23: cherry-picked onto `main`** by the housekeeping PR + ([#35](https://github.com/NetDevAutomate/StudyLoop/pull/35)); `feat/learning-tier-fed` branches from + `main` with the plan already on it. diff --git a/docs/architecture/learning-tier/receipts/s1-0-capability-lock.md b/docs/architecture/learning-tier/receipts/s1-0-capability-lock.md new file mode 100644 index 000000000..f46fc4e6e --- /dev/null +++ b/docs/architecture/learning-tier/receipts/s1-0-capability-lock.md @@ -0,0 +1,91 @@ +# S1-0 — Capability lock (learning tier, item 1) + +**Date:** 2026-09-23 · **Tree:** `main` @ `c1a28de1` (decisions recorded on PR #36, head `046fa499`) · +**Issue:** #38 · **Plan:** `docs/architecture/learning-tier/plan-2026-09-19.md` §3 S1-0 · **Branch:** `feat/learning-tier-fed` + +This receipt records, from source, how each of the six supported harnesses grants tool use to the +mentor today, and freezes the two writer sets item 1 builds against. It is the S1-0 finish line: +nothing here is a change; S1-RED and S1-GREEN cite it instead of re-deriving it. Every claim carries +the file and line it was read from on the tree above. + +## 1. Writer sets (frozen) + +| Set | Tools | Character | Rule this branch (owner decision 2, plan §7) | +|---|---|---|---| +| `W_auto` | `log_topic` (`mcp/tools.py:583`), `log_struggle` (`:850`), `record_plan_learning` (`:131`), **`record_teachback` (new — no MCP tool exists; the CLI writer is `history/teachback.py:27`)** | Additive: each appends a record the learner agreed to or stated | **Named in every harness definition; prompt-per-call everywhere; no new pre-approval on any harness.** | +| `W_srs` | `record_study_progress` (`mcp/tools.py:114`), `record_topic_progress` (`:538`), `log_review_outcome` (`:648`) | SRS mutators: reschedule a card or resolve a topic on an id nothing verifies (`card_hash`, backlog `topic_id`) | Prompt-per-call, unchanged, not named by any trigger. | + +Readers the definitions already name (`get_concept_context`, `get_study_history`, `get_next_action`, +`get_topic_suggestions`, `get_active_topics`, `read_lesson`, …) are outside both sets and unchanged. + +## 2. Grant mechanism per harness (as it is) + +| Harness | Definition file(s) | Grant grammar in repo | Approval cell today | S1-GREEN changes (instruction only) | +|---|---|---|---|---| +| **Kiro CLI** | `agents/kiro/study-mentor.json`; persona `agents/kiro/study-mentor/persona.md` | `tools` (`:13`) exposes `@builtin @study-speak @session-db @studyloop`; `allowedTools` (`:50-68`) pre-approves `execute_bash` (`:52`), five `@studyloop/get_*` readers (`:63-67`) and one writer, `@studyloop/log_topic` (`:68`) | `log_topic` pre-approved; the other three `W_auto` tools prompt; **CLI writers (`studyloop progress`, `studyloop teachback`) run un-prompted through `execute_bash`** | Persona repointed (`persona.md:32` and `:81` route "record progress" to `uv run tutor-checkpoint`, a different tool); trigger table projected. `allowedTools` **unchanged** — `log_topic` stays as the one recorded asymmetry. | +| **Claude Code** | `agents/claude/socratic-mentor.md`; `agents/claude/settings.json` | Sub-agent frontmatter `tools:` (`socratic-mentor.md:5`) = `Read, Write, Grep, Bash` — **no `mcp__studyloop__*` tool named**; `settings.json` has a single key, `statusLine` — **no `permissions` block**. The sibling `study-plan-architect.md:5` shows the naming grammar (ten `mcp__studyloop__*` tools). | Every tool call prompts; the mentor cannot call any `studyloop` MCP tool at all because none is named — it reaches the learning tier only through `Bash` and the CLI | `tools:` line names each `W_auto` tool and the readers (naming is instruction, not approval). **No `permissions` block is added.** | +| **OpenCode** | `agents/opencode/study-mentor.md` | `permission:` (`:10-16`): `edit: allow`; `bash:` `"studyloop *": allow`, `"session-* *": allow`, `"uv run tutor-*": allow`, `"*": ask` | CLI writers un-prompted through the bash wildcard (wider than least privilege — every `studyloop` verb); MCP tool approval is the harness default | Persona names each `W_auto` tool; **wildcard unchanged** (recorded here as known, not least-privilege, out of scope for this branch). | +| **Codex** | `agents/codex/AGENTS.md` (canonical persona, written into the session dir by `adapters/codex.py:20-27`) | **None in repo.** Codex reads `AGENTS.md` from cwd; approval is Codex's own policy | Harness-side | Canonical persona names each `W_auto` tool via the shared protocol; nothing else. | +| **pi** | `agents/pi/AGENTS.md` (+ `agents/pi/extensions/studyloop-session-export.ts`); written by `adapters/pi.py:15-16` | **None in repo.** | Harness-side | As Codex. | +| **Grok Build** | no `agents/grok/` directory (by design — `adapters/grok.py:97-101` writes `AGENTS.md` into the session dir; `:17` records that no Grok permission — `ui.yolo`, tool approval, hooks trust — is touched) | **None in repo.** | Harness-side | As Codex. No `agents/grok/` is created. | + +Parity, therefore, is of the **instruction** — one canonical `agents/shared/recording-protocol.md` with a +fenced YAML trigger table, projected byte-identically into every definition above and hashed in +`agents/manifest.json` — and of the **names**: every definition names the same `W_auto` set. It is never +parity of approval: three of the six harnesses cannot express approval in this repository at all. + +## 3. The writer that does not exist yet — its contract, read from the CLI + +`record_teachback` (MCP) must validate exactly as `cli/_teachback.py` does and land through the same +function: + +- Scores: exactly five integers, each 1–4 (`cli/_teachback.py:16-46`), in the order + `(accuracy, own_words, structure, depth, transfer)` (`history/teachback.py:27-42` docstring). +- `review_type` ∈ `TEACHBACK_TYPES = ("micro", "structured", "transfer", "full")` (`cli/_teachback.py:12`). +- `angle` and `notes` optional strings. +- Writer: `history.teachback.record_teachback(concept, topic, scores, review_type, angle, notes, + session_id) -> bool`; one row in `teach_back_scores`, whose five score columns carry + `CHECK(... BETWEEN 1 AND 4)` (`agent_session_tools/migrations.py:504-513`); `False` means no + connection, and the MCP tool must surface that as a `ToolError`, not a silent success. +- `session_id` is bound from the live session state, the way `end_session` reads it + (`mcp/tools.py:529-531`, `read_session_state()["study_session_id"]`), never from the caller. +- Teach-backs are events: a repeated call is two rows, documented as such (plan §5). + +## 4. What is *not* in this receipt, on purpose + +- No grant is widened or narrowed. The only edits S1-GREEN makes to definition files are names and the + projected protocol. +- The noticing episode (decision 3) is scheduled by rule — the first real study session after S1-GREEN + reaches `main` — and its date is written into the S1-SIM receipt, not here. +- Item 2 (Jev) is not on this branch (decision 1; #39). + +## 5. Addendum at S1-GREEN (2026-09-23) — two corrections to §2, recorded rather than erased + +**(a) The live session persona is not the installed definition.** §2 says the Codex/pi/Grok adapters +write "the canonical persona" into the session dir and cites `agents/codex/AGENTS.md`. Verified at GREEN +(while checking council review 9's grok Y2): `studyloop study` calls +`agent_launcher.build_canonical_persona(mode, …)`, which renders **`agents/shared/personas/.md`** +(`study.md`, `co-study.md`, `plan-architect.md`) and hands that string to *every* adapter's `setup()` — +Kiro's agent prompt, Claude's flag file, and the session-dir `AGENTS.md` for Codex, pi and Grok Build +alike. The files in §2 are the harnesses' *installed* definitions (global steering / sub-agent), read +when a learner opens the harness directly. So a writer named only in §2's files is unnamed in every live +session. GREEN therefore names the four writers and references the protocol in `study.md` and +`co-study.md` as well, and `test_adapter_parity.py::test_the_built_live_persona_names_each_writer` pins +the **built** string, not a file. Before this branch, `study.md` told the mentor to *show the learner a +`studyloop topic` command* and never to write anything itself — the defect in its exact live form. + +**(b) "Projected byte-identically" overstated the mechanism.** The protocol is ONE file, +`agents/shared/recording-protocol.md`, hashed in `agents/manifest.json`; each definition and each live +persona carries an identical one-sentence *reference* to it and names the four writers — the same +mechanism every sibling protocol (`teach-back-protocol.md`, `wind-down-protocol.md`, …) already uses. +Distribution of `agents/shared/` is the installer's existing link (`installers.py:53` → +`~/.kiro/agents/shared`, `:128` → `~/.agents/shared` for pi). **UNVERIFIED:** whether a Codex, Claude +Code or OpenCode mentor launched outside the repository resolves a repo-relative `agents/shared/…` +reference — a pre-existing question for all nine shared protocols, listed for S1-SIM (council review 9, +astra Y3). Nothing here inlines the table into each definition; naming plus the reference is the +instruction parity decision 2 asks for. + +**S1-0 finish:** this file committed. Next: S1-RED (plan §5 table) — `test_mcp_teachback.py` (tool +absent), `test_adapter_parity.py` extended (names absent, no new grants), +`test_docs_harness_tier_contract.py` extended (protocol absent; `persona.md:32` still names +`tutor-checkpoint`), `test_writer_isolation.py` (guard if it already passes), no-trigger/duplicate replay. diff --git a/docs/contributing.md b/docs/contributing.md index 6577a5cd6..d47376dad 100644 --- a/docs/contributing.md +++ b/docs/contributing.md @@ -125,7 +125,10 @@ harness's transcripts). matter in six months, write an ADR (`docs/adr/NNNN-kebab-title.md`, indexed in `docs/adr/README.md`). - *New mentor harness or backend*: the issue must define the persona mechanism, launch and resume commands, session store, exporter, health check, failure behaviour and a live acceptance path. Drive-by - harness additions are declined. + harness additions are declined. A mentor definition must also reference + `agents/shared/recording-protocol.md` and name its four writers (`record_teachback`, `log_struggle`, + `log_topic`, `record_plan_learning`) — `test_adapter_parity.py` and `test_docs_harness_tier_contract.py` + pin this, and pin that no harness gains a pre-approval for them (owner decision, 2026-09-23). 2. **Fork and branch.** Outside contributors fork and open pull requests against `main`; maintainers branch in the repository. Name branches `feat/…`, `fix/…`, `docs/…` or `test/…`. Do not use `lane/…`: that prefix is reserved for the maintainers' remediation lanes and carries an ownership test that only makes diff --git a/packages/studyloop/src/studyloop/cli/_teachback.py b/packages/studyloop/src/studyloop/cli/_teachback.py index 04cc98a92..9eb2a00aa 100644 --- a/packages/studyloop/src/studyloop/cli/_teachback.py +++ b/packages/studyloop/src/studyloop/cli/_teachback.py @@ -8,12 +8,18 @@ from rich.table import Table from studyloop.cli._shared import console +from studyloop.history.teachback import TEACHBACK_TYPES, coerce_scores -TEACHBACK_TYPES = ("micro", "structured", "transfer", "full") +__all__ = ["TEACHBACK_TYPES", "teachback"] class TeachbackScoresParam(click.ParamType): - """Parse five comma-separated teach-back rubric scores.""" + """Parse five comma-separated teach-back rubric scores. + + The rule itself lives in :func:`studyloop.history.teachback.coerce_scores`, + shared with the ``record_teachback`` MCP tool; this class only splits the + shell string and maps the shared ``ValueError`` to click's failure. + """ name = "scores" @@ -26,24 +32,10 @@ def convert( if isinstance(value, tuple): return cast("tuple[int, int, int, int, int]", value) - raw = str(value) - parts = [part.strip() for part in raw.split(",")] - if len(parts) != 5 or any(part == "" for part in parts): - self.fail( - 'expected exactly five comma-separated scores, e.g. "3,3,4,3,2"', - param, - ctx, - ) - try: - scores = tuple(int(part) for part in parts) - except ValueError: - self.fail("scores must be integers from 1 to 4", param, ctx) - - if any(score < 1 or score > 4 for score in scores): - self.fail("each score must be between 1 and 4", param, ctx) - - return cast("tuple[int, int, int, int, int]", scores) + return coerce_scores(str(value).split(",")) + except ValueError as exc: + self.fail(str(exc), param, ctx) TEACHBACK_SCORES = TeachbackScoresParam() diff --git a/packages/studyloop/src/studyloop/history/teachback.py b/packages/studyloop/src/studyloop/history/teachback.py index a7141cc1f..28b09aeb6 100644 --- a/packages/studyloop/src/studyloop/history/teachback.py +++ b/packages/studyloop/src/studyloop/history/teachback.py @@ -6,13 +6,57 @@ import logging import sqlite3 from datetime import UTC, datetime +from typing import TYPE_CHECKING, cast from agent_session_tools.context import records from . import _connection, observations +if TYPE_CHECKING: + from collections.abc import Sequence + logger = logging.getLogger(__name__) +#: Review types a teach-back may be recorded under (CLI ``--type`` and MCP ``review_type``). +TEACHBACK_TYPES = ("micro", "structured", "transfer", "full") + +#: Rubric order of the five scores. +SCORE_DIMENSIONS = ("accuracy", "own_words", "structure", "depth", "transfer") + +_FIVE_SCORES = 'expected exactly five comma-separated scores, e.g. "3,3,4,3,2"' +_INTEGERS = "scores must be integers from 1 to 4" +_RANGE = "each score must be between 1 and 4" + + +def coerce_scores(values: Sequence[object]) -> tuple[int, int, int, int, int]: + """Validate five rubric scores the one way both surfaces share. + + The CLI (``studyloop teachback --score``) and the MCP tool + (``record_teachback``) accept scores from different callers -- a shell + string and a JSON list -- but the rule is one: exactly five, integers, + each 1 to 4, in :data:`SCORE_DIMENSIONS` order. Raises ``ValueError`` + with the message the CLI has always printed; each surface maps that to + its own error type. + """ + parts = [str(part).strip() for part in values] + if len(parts) != 5 or any(part == "" for part in parts): + raise ValueError(_FIVE_SCORES) + try: + scores = tuple(int(part) for part in parts) + except ValueError as exc: + raise ValueError(_INTEGERS) from exc + if any(score < 1 or score > 4 for score in scores): + raise ValueError(_RANGE) + return cast("tuple[int, int, int, int, int]", scores) + + +def coerce_review_type(value: str) -> str: + """Validate ``review_type`` against :data:`TEACHBACK_TYPES`; ``ValueError`` names the set.""" + if value not in TEACHBACK_TYPES: + allowed = ", ".join(TEACHBACK_TYPES) + raise ValueError(f"review type must be one of {allowed}; got {value!r}") + return value + def _confidence_from_teachback(total: int, review_type: str) -> str: if total < 9: diff --git a/packages/studyloop/src/studyloop/mcp/tools.py b/packages/studyloop/src/studyloop/mcp/tools.py index e94d6b44f..8c2a23064 100644 --- a/packages/studyloop/src/studyloop/mcp/tools.py +++ b/packages/studyloop/src/studyloop/mcp/tools.py @@ -864,6 +864,80 @@ def log_struggle( row_id = park_topic(question, topic_tag=topic_tag, context=context, source="struggled") return {"status": "logged", "id": row_id} + @tool() + def record_teachback( + concept: str, + topic: str, + scores: list[int], + review_type: str, + angle: str = "", + notes: str = "", + ) -> dict[str, Any]: + """Record a teach-back score the learner has agreed to. + + Call once a teach-back round has ended and the learner has accepted + the five proposed scores (``agents/shared/recording-protocol.md``, + trigger ``teach_back_agreed``). Scores are in rubric order -- + accuracy, own_words, structure, depth, transfer -- each 1 to 4; + ``review_type`` is one of micro, structured, transfer, full. + Teach-backs are events: a repeated call records a second row. + + The row is owned by the configured scope, exactly as the CLI's rows + are. The live study session id is returned for the caller's record + but is not a storage link: the ownership layer binds a ``session_id`` + only to a native harness session and links study sessions only for + parked topics and study notes (S1-0 receipt, finding N1). + + Args: + concept: The concept that was taught back. + topic: Study topic (python, sql, ...). + scores: Exactly five integers, each 1-4, in rubric order. + review_type: micro, structured, transfer or full. + angle: Optional question angle used (e.g. "apply_network_analogy"). + notes: Optional assessment notes. + """ + from studyloop.history.teachback import coerce_review_type, coerce_scores + from studyloop.history.teachback import record_teachback as write_teachback + from studyloop.session_state import read_session_state + + try: + five = coerce_scores(scores) + kind = coerce_review_type(review_type) + except ValueError as exc: + raise ToolError(str(exc)) from exc + concept = concept.strip() + topic = topic.strip() + if not concept or not topic: + # NOT NULL in the schema does not stop "" -- refuse it here so a blank + # never becomes a row nothing can find again (council review 9, grok Y5). + raise ToolError("concept and topic must both be non-empty") + + study_session_id = read_session_state().get("study_session_id") or None + recorded = write_teachback( + concept=concept, + topic=topic, + scores=five, + review_type=kind, + angle=angle or None, + notes=notes or None, + ) + if not recorded: + # The writer returns False for three different failures -- no database, + # a lock/timeout, a refused row -- and does not say which (its CLI caller + # inherited the same collapse). Say that, rather than claim one cause. + raise ToolError( + "teach-back not recorded: the sessions database could not take the write " + "(unavailable, locked, or it refused the row) -- run `studyloop doctor`" + ) + return { + "recorded": True, + "concept": concept, + "topic": topic, + "review_type": kind, + "total": sum(five), + "study_session_id": study_session_id, + } + # ── Study plans — discovery, authoring and progression through the seam (D-4, D-8, D-9) ── # # Nine thin adapters over ``studyloop.planning.PlanApplication`` (design §4): diff --git a/packages/studyloop/src/studyloop/web/routes/session/_ws.py b/packages/studyloop/src/studyloop/web/routes/session/_ws.py index 4ed740110..175147d36 100644 --- a/packages/studyloop/src/studyloop/web/routes/session/_ws.py +++ b/packages/studyloop/src/studyloop/web/routes/session/_ws.py @@ -161,7 +161,16 @@ async def pty_to_ws() -> None: {"type": "agent_message", "kind": event.kind, "payload": event.payload} ) finally: - if not nxt.done(): + if nxt.done(): + # A pull that completed after the loop's last read -- a drain + # (StopAsyncIteration) landing beside a takeover, or a client + # close that cancelled this coroutine while the pull was + # already done. Read its outcome so asyncio does not log + # "Task exception was never retrieved" from the finalizer; + # a drained stream ending here is expected, not an error. + if not nxt.cancelled(): + nxt.exception() + else: nxt.cancel() # Only the PTY transport's events() is an async generator, so only # it has aclose(). The ACP transport deliberately returns a diff --git a/packages/studyloop/tests/test_adapter_parity.py b/packages/studyloop/tests/test_adapter_parity.py index 29f412741..3529d0afa 100644 --- a/packages/studyloop/tests/test_adapter_parity.py +++ b/packages/studyloop/tests/test_adapter_parity.py @@ -90,3 +90,139 @@ def test_readme_names_every_supported_harness_by_its_label() -> None: if not re.search(rf"\b{re.escape(harness.label)}\b", text) ] assert not missing, f"README.md does not name these supported harnesses: {missing}" + + +# --------------------------------------------------------------------------- +# Learning tier, item 1 (S1-RED): the writers are NAMED everywhere, GRANTED nowhere new +# --------------------------------------------------------------------------- +# +# Owner decision 2 (docs/architecture/learning-tier/plan-2026-09-19.md §7): the +# additive writers are named in every harness definition and stay prompt-per-call; +# no harness gains a pre-approval on this branch. Parity is of the instruction, +# never of approval -- codex, pi and grok cannot express approval in this repo. +# The S1-0 receipt (docs/architecture/learning-tier/receipts/s1-0-capability-lock.md) +# records the cells these tests pin. + +#: The additive, learner-agreed writers (``record_teachback`` is the new one). +W_AUTO = ("log_topic", "log_struggle", "record_teachback", "record_plan_learning") + +#: The mentor definition each harness INSTALLS (global steering / sub-agent). Grok Build +#: reads the same canonical file as Codex (its header says so; ``adapters/grok.py`` +#: projects it). The LIVE session persona is a different document -- see +#: ``LIVE_PERSONAS`` and ``test_the_built_live_persona_names_each_writer`` below. +MENTOR_DEFINITIONS = { + "kiro": "agents/kiro/study-mentor/persona.md", + "claude": "agents/claude/socratic-mentor.md", + "opencode": "agents/opencode/study-mentor.md", + "codex": "agents/codex/AGENTS.md", + "pi": "agents/pi/AGENTS.md", + "grok": "agents/codex/AGENTS.md", +} + +#: What ``studyloop study`` actually hands EVERY adapter's ``setup()`` (council review 9, +#: coordinator finding): ``agent_launcher.build_canonical_persona(mode, ...)`` renders +#: ``agents/shared/personas/.md``, not the installed definition above. A writer named +#: only in the installed files would be unnamed in every live session. +LIVE_PERSONAS = ("agents/shared/personas/study.md", "agents/shared/personas/co-study.md") + + +def _definition(harness: str) -> str: + return (REPO_ROOT / MENTOR_DEFINITIONS[harness]).read_text(encoding="utf-8") + + +def test_mentor_definitions_cover_exactly_the_six() -> None: + assert set(MENTOR_DEFINITIONS) == EXPECTED_HARNESSES + + +def test_every_mentor_definition_names_each_w_auto_writer() -> None: + """A writer the definition never names is a writer the mentor never calls.""" + missing = { + harness: [w for w in W_AUTO if not re.search(rf"\b{w}\b", _definition(harness))] + for harness in sorted(MENTOR_DEFINITIONS) + } + missing = {h: ws for h, ws in missing.items() if ws} + assert not missing, f"W_auto writers not named: {missing}" + + +def test_every_live_persona_names_each_writer_and_the_protocol() -> None: + for rel in LIVE_PERSONAS: + text = (REPO_ROOT / rel).read_text(encoding="utf-8") + missing = [w for w in W_AUTO if not re.search(rf"\b{w}\b", text)] + assert not missing, f"{rel} does not name {missing}" + assert "recording-protocol.md" in text, f"{rel} does not reference the protocol" + + +def test_the_built_live_persona_names_each_writer(monkeypatch) -> None: + """Pin the projection, not the file: the string every adapter's ``setup()`` receives.""" + from studyloop import session_state + from studyloop.agent_launcher import build_canonical_persona + + monkeypatch.setattr(session_state, "STATE_FILE", Path("/fixed/session-state.json")) + monkeypatch.setattr(session_state, "TOPICS_FILE", Path("/fixed/session-topics.md")) + monkeypatch.setattr(session_state, "PARKING_FILE", Path("/fixed/session-parking.md")) + for mode in ("study", "co-study"): + built = build_canonical_persona(mode, "window functions", 5) + missing = [w for w in W_AUTO if not re.search(rf"\b{w}\b", built)] + assert not missing, f"built {mode} persona does not name {missing}" + assert "recording-protocol.md" in built + + +def test_grok_projects_the_codex_definition() -> None: + """``MENTOR_DEFINITIONS['grok']`` is the Codex file because the Grok adapter says so; + pin that statement so an ``agents/grok/`` split cannot drop the protocol silently.""" + header = (REPO_ROOT / "packages/studyloop/src/studyloop/adapters/grok.py").read_text( + encoding="utf-8" + ) + assert "AGENTS.md" in header.split('"""')[1], "grok adapter no longer documents AGENTS.md" + assert not (REPO_ROOT / "agents/grok").exists(), ( + "agents/grok/ now exists: add it to MENTOR_DEFINITIONS and the protocol tests" + ) + + +def test_claude_mentor_tools_line_names_each_writer() -> None: + """Claude's sub-agent frontmatter restricts tools to the ``tools:`` line; + an unnamed MCP tool is unreachable there, whatever the permissions say.""" + text = _definition("claude") + match = re.search(r"^tools:\s*(.+)$", text, flags=re.MULTILINE) + assert match, "socratic-mentor.md has no frontmatter tools: line" + named = {item.strip() for item in match.group(1).split(",")} + missing = [w for w in W_AUTO if f"mcp__studyloop__{w}" not in named] + assert not missing, f"tools: line does not name {missing}; it names {sorted(named)}" + + +def test_kiro_gains_no_pre_approval() -> None: + """The one recorded asymmetry stays exactly one: ``log_topic`` -- and no wildcard + (``@studyloop`` or ``@studyloop/*``) can grant the rest by another spelling.""" + import json + + spec = json.loads((REPO_ROOT / "agents/kiro/study-mentor.json").read_text(encoding="utf-8")) + allowed = set(spec["allowedTools"]) + assert allowed & {f"@studyloop/{w}" for w in W_AUTO} == {"@studyloop/log_topic"} + wildcards = {a for a in allowed if a in {"@studyloop", "@studyloop/*"} or a.endswith("*")} + assert wildcards == set(), f"allowedTools carries a wildcard grant: {sorted(wildcards)}" + + +def test_claude_settings_grant_nothing() -> None: + import json + + settings = json.loads((REPO_ROOT / "agents/claude/settings.json").read_text(encoding="utf-8")) + assert "permissions" not in settings, "decision 2: no permissions block on Claude" + + +def test_opencode_permission_block_is_exactly_as_recorded() -> None: + """Recorded as known and not least-privilege in the S1-0 receipt; unchanged here -- + parsed, not substring-matched, so an added allow rule fails too.""" + import yaml + + text = _definition("opencode") + frontmatter = text.split("---")[1] + permission = yaml.safe_load(frontmatter)["permission"] + assert permission == { + "edit": "allow", + "bash": { + "studyloop *": "allow", + "session-* *": "allow", + "uv run tutor-*": "allow", + "*": "ask", + }, + }, permission diff --git a/packages/studyloop/tests/test_docs_harness_tier_contract.py b/packages/studyloop/tests/test_docs_harness_tier_contract.py index 07d3ea884..aa4243a97 100644 --- a/packages/studyloop/tests/test_docs_harness_tier_contract.py +++ b/packages/studyloop/tests/test_docs_harness_tier_contract.py @@ -18,6 +18,7 @@ import re from pathlib import Path +from typing import ClassVar from studyloop.harnesses import CORE_HARNESSES, HARNESSES, PREVIEW_HARNESSES, RELEASE_HARNESSES @@ -162,3 +163,84 @@ class TestHarnessRecordsAgreeWithTiers: def test_core_flag_mirrors_the_tuples(self) -> None: assert {n for n, h in HARNESSES.items() if h.core} == set(CORE_HARNESSES) assert {n for n, h in HARNESSES.items() if not h.core} == set(PREVIEW_HARNESSES) + + +# --------------------------------------------------------------------------- +# Learning tier, item 1 (S1-RED): one recording protocol, projected to every harness +# --------------------------------------------------------------------------- + + +class TestRecordingProtocol: + """``agents/shared/recording-protocol.md`` is the one instruction that tells + every mentor WHEN to write. Plan §5; owner decision 2 (parity of instruction). + """ + + PROTOCOL = "agents/shared/recording-protocol.md" + W_AUTO = ("log_topic", "log_struggle", "record_teachback", "record_plan_learning") + DEFINITIONS = ( + "agents/kiro/study-mentor/persona.md", + "agents/claude/socratic-mentor.md", + "agents/opencode/study-mentor.md", + "agents/codex/AGENTS.md", + "agents/pi/AGENTS.md", + # The LIVE personas: ``build_canonical_persona`` renders these into every + # adapter's session document, whatever the installed definition says. + "agents/shared/personas/study.md", + "agents/shared/personas/co-study.md", + ) + REQUIRED_KEYS: ClassVar[frozenset[str]] = frozenset( + {"trigger", "writer", "required_ids", "consent"} + ) + + def _table(self) -> list[dict]: + import yaml + + text = _read(self.PROTOCOL) + blocks = re.findall(r"^```yaml\s*\n(.*?)^```", text, flags=re.MULTILINE | re.DOTALL) + assert len(blocks) == 1, f"expected exactly one fenced yaml block, found {len(blocks)}" + table = yaml.safe_load(blocks[0]) + assert isinstance(table, list) and table, "the trigger table is a non-empty YAML list" + return table + + def test_the_protocol_exists(self) -> None: + assert (REPO_ROOT / self.PROTOCOL).is_file(), f"{self.PROTOCOL} is absent" + + def test_the_trigger_table_parses_with_the_documented_columns(self) -> None: + for row in self._table(): + assert set(row) >= self.REQUIRED_KEYS, ( + f"row lacks {self.REQUIRED_KEYS - set(row)}: {row}" + ) + + def test_every_trigger_names_a_w_auto_writer_and_every_writer_has_a_trigger(self) -> None: + writers = [row["writer"] for row in self._table()] + strangers = [w for w in writers if w not in self.W_AUTO] + assert not strangers, f"triggers name writers outside W_auto: {strangers}" + untriggered = [w for w in self.W_AUTO if w not in writers] + assert not untriggered, f"W_auto writers with no trigger: {untriggered}" + + def test_no_trigger_names_an_srs_mutator(self) -> None: + srs = {"record_study_progress", "log_review_outcome", "record_topic_progress"} + assert not {row["writer"] for row in self._table()} & srs + + def test_every_mentor_definition_references_the_protocol(self) -> None: + missing = [d for d in self.DEFINITIONS if "recording-protocol.md" not in _read(d)] + assert not missing, f"definitions that do not reference the protocol: {missing}" + + def test_the_manifest_hashes_the_protocol(self) -> None: + import hashlib + import json + + manifest = json.loads(_read("agents/manifest.json")) + entry = manifest["agents"].get("shared/recording-protocol.md") + assert entry, "agents/manifest.json has no entry for shared/recording-protocol.md" + digest = hashlib.sha256((REPO_ROOT / self.PROTOCOL).read_bytes()).hexdigest()[:16] + assert entry["hash"] == digest, "manifest hash is stale for the protocol" + + def test_the_kiro_persona_no_longer_routes_record_progress_to_tutor_checkpoint(self) -> None: + lines = _read("agents/kiro/study-mentor/persona.md").splitlines() + offenders = [ + ln for ln in lines if "record progress" in ln.lower() and "tutor-checkpoint" in ln + ] + assert offenders == [], ( + f"persona still routes record-progress to tutor-checkpoint: {offenders}" + ) diff --git a/packages/studyloop/tests/test_e2e_coverage_gate_selftest.py b/packages/studyloop/tests/test_e2e_coverage_gate_selftest.py index 3bfefc714..0adc63880 100644 --- a/packages/studyloop/tests/test_e2e_coverage_gate_selftest.py +++ b/packages/studyloop/tests/test_e2e_coverage_gate_selftest.py @@ -16,6 +16,14 @@ import pytest +# ``test_gate_fails_when_a_new_route_is_untested`` scans every test file for route +# references twice (route test + full-stack test): 26 s on a laptop, and on +# 2026-09-23 it crossed the 60 s unit ceiling under ``--cov`` on a shared CI runner +# (python 3.13 lane; 3.12 passed; green on rerun). pyproject.toml's policy is that a +# module whose honest cost is near the ceiling carries its own bound rather than +# flaking against the global one. +pytestmark = pytest.mark.timeout(180) + _tests_dir = str(Path(__file__).resolve().parent) if _tests_dir not in sys.path: sys.path.insert(0, _tests_dir) diff --git a/packages/studyloop/tests/test_mcp_plan_tools.py b/packages/studyloop/tests/test_mcp_plan_tools.py index f37ddd990..57b93e0b6 100644 --- a/packages/studyloop/tests/test_mcp_plan_tools.py +++ b/packages/studyloop/tests/test_mcp_plan_tools.py @@ -97,8 +97,9 @@ #: 23 original tools at ``0a20a796`` — ``record_plan_learning`` among them — #: plus the nine design-§4 plan tools = 32 (council review 3, F13: the design's #: "35" was arithmetic on a stale inventory; review 4, F6: the earlier comment -#: here said "plus nine less one", which is 31). -PRODUCTION_TOOL_COUNT = 32 +#: here said "plus nine less one", which is 31), plus ``record_teachback`` +#: (learning tier item 1, 2026-09-23) = 33. +PRODUCTION_TOOL_COUNT = 33 #: The core names the stdio smoke test also pins; asserted here too so the #: in-process twin is a real twin (review 4, F4). @@ -933,8 +934,8 @@ def test_phase_four_schemas_carry_the_design_signatures() -> None: assert "const" not in confirmed and "enum" not in confirmed -def test_production_inventory_is_thirty_two_with_the_nine_plan_tools() -> None: - """The in-process twin of the stdio pin (T4.1): exactly 32 names, each +def test_production_inventory_is_thirty_three_with_the_nine_plan_tools() -> None: + """The in-process twin of the stdio pin (T4.1): exactly 33 names, each tool registered under its own name (a duplicate registration would overwrite its key silently, so the dict's size alone cannot show one), all nine design-§4 plan tools, ``record_plan_learning`` and the core diff --git a/packages/studyloop/tests/test_mcp_stdio_smoke.py b/packages/studyloop/tests/test_mcp_stdio_smoke.py index 1490a5979..b9561fd53 100644 --- a/packages/studyloop/tests/test_mcp_stdio_smoke.py +++ b/packages/studyloop/tests/test_mcp_stdio_smoke.py @@ -42,10 +42,11 @@ #: ``record_plan_learning`` among them — plus the nine plan tools = 32 #: (council review 3, F13 — the design's "26 → 35" was arithmetic on a stale #: count; review 4, F6 — an earlier form of this comment said "plus nine less -#: one", which is 31). +#: one", which is 31), plus ``record_teachback`` (learning tier item 1, +#: 2026-09-23) = 33. #: Exact, not a lower bound: an accidental registration is a failure here, and #: the name assertions stop an unrelated addition masking a missing tool. -PRODUCTION_TOOL_COUNT = 32 +PRODUCTION_TOOL_COUNT = 33 @pytest.fixture diff --git a/packages/studyloop/tests/test_mcp_teachback.py b/packages/studyloop/tests/test_mcp_teachback.py new file mode 100644 index 000000000..d9e1d9ff9 --- /dev/null +++ b/packages/studyloop/tests/test_mcp_teachback.py @@ -0,0 +1,226 @@ +"""RED for the ``record_teachback`` MCP tool (learning tier, item 1, stage S1-RED). + +The learning tier is unfed because the mentor has no MCP writer for a teach-back +score: the only writer is the CLI (``studyloop teachback``), which an agent +session never calls. Plan §5 (``docs/architecture/learning-tier/plan-2026-09-19.md``) +and the S1-0 receipt pin the contract this file tests: + +* the tool exists beside the other ``W_auto`` writers; +* it validates exactly as ``cli/_teachback.py`` does -- five scores, integers, + each 1-4, ``review_type`` in ``TEACHBACK_TYPES`` -- and lands **no row** when + validation fails; +* a valid call lands **one row** in ``teach_back_scores`` through the real + migrated schema, whose CHECK constraints are what "honouring CHECK" means; +* the live study session id is reported in the reply and never taken from + the caller; it is not stored as the row's ``session_id`` (finding N1: the + ownership layer reserves that column for native harness sessions); +* a repeated call is two rows -- teach-backs are events, not state; +* a missing connection is a ``ToolError``, never a silent success. + +Accesses the tool via the FastMCP registry, mirroring ``test_mcp_log_topic.py``. +The scratch database comes from ``STUDYLOOP_DB`` (read at call time), so the +schema is the one ``_connection._connect`` migrates, not a hand-rolled table. +""" + +from __future__ import annotations + +import inspect +import sqlite3 +from typing import TYPE_CHECKING + +import pytest + +pytest.importorskip("mcp") + +from mcp.server.fastmcp.exceptions import ToolError + +from studyloop.mcp.server import mcp + +if TYPE_CHECKING: + from pathlib import Path + +W_AUTO = ("log_topic", "log_struggle", "record_teachback", "record_plan_learning") +VALID_SCORES = [3, 3, 4, 3, 2] + + +def _get_tool(name: str): + tools = mcp._tool_manager._tools + if name not in tools: + raise KeyError(f"Tool {name!r} not found. Available: {sorted(tools)}") + return tools[name].fn + + +def _rows(db: Path) -> list[tuple]: + conn = sqlite3.connect(db) + try: + return conn.execute( + "SELECT concept, topic, session_id, score_accuracy, score_own_words, " + "score_structure, score_depth, score_transfer FROM teach_back_scores ORDER BY id" + ).fetchall() + except sqlite3.OperationalError as exc: # table absent: nothing was ever written + if "no such table" in str(exc): + return [] + raise + finally: + conn.close() + + +@pytest.fixture() +def scratch_db(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + """A per-test sessions.db that the real connection resolver migrates.""" + db = tmp_path / "sessions.db" + monkeypatch.setenv("STUDYLOOP_DB", str(db)) + return db + + +@pytest.fixture() +def session_state(tmp_path: Path, monkeypatch: pytest.MonkeyPatch): + """Redirect the session-state files into tmp_path and return the writer.""" + from studyloop import session_state as ss + + monkeypatch.setattr(ss, "SESSION_DIR", tmp_path) + monkeypatch.setattr(ss, "STATE_FILE", tmp_path / "session-state.json") + monkeypatch.setattr(ss, "TOPICS_FILE", tmp_path / "session-topics.md") + monkeypatch.setattr(ss, "PARKING_FILE", tmp_path / "session-parking.md") + return ss.write_session_state + + +class TestTheToolExists: + def test_record_teachback_is_registered_beside_the_other_writers(self) -> None: + registered = set(mcp._tool_manager._tools) + missing = [name for name in W_AUTO if name not in registered] + assert missing == [], f"W_auto writers absent from the MCP registry: {missing}" + + def test_the_caller_cannot_supply_a_session_id(self) -> None: + params = inspect.signature(_get_tool("record_teachback")).parameters + assert "session_id" not in params, "session_id is bound from session state, not the caller" + + +class TestValidationMirrorsTheCli: + """Every rejection is a ToolError and leaves the table untouched.""" + + @pytest.mark.parametrize( + ("scores", "fragment"), + [ + ([3, 3, 4, 3], "five"), + ([3, 3, 4, 3, 2, 1], "five"), + ([0, 3, 4, 3, 2], "1 and 4"), + ([3, 3, 4, 3, 5], "1 and 4"), + ], + ) + def test_score_shape_and_range(self, scratch_db: Path, scores: list, fragment: str) -> None: + tool = _get_tool("record_teachback") + with pytest.raises(ToolError, match=fragment): + tool(concept="window frame", topic="sql", scores=scores, review_type="micro") + assert _rows(scratch_db) == [] + + def test_non_integer_scores_are_rejected(self, scratch_db: Path) -> None: + tool = _get_tool("record_teachback") + with pytest.raises(ToolError, match="integer"): + tool( + concept="window frame", topic="sql", scores=["3", "x", 4, 3, 2], review_type="micro" + ) + assert _rows(scratch_db) == [] + + def test_unknown_review_type_is_rejected_naming_the_allowed_set(self, scratch_db: Path) -> None: + from studyloop.cli._teachback import TEACHBACK_TYPES + + tool = _get_tool("record_teachback") + with pytest.raises(ToolError) as excinfo: + tool(concept="window frame", topic="sql", scores=VALID_SCORES, review_type="vibes") + for allowed in TEACHBACK_TYPES: + assert allowed in str(excinfo.value) + assert _rows(scratch_db) == [] + + @pytest.mark.parametrize(("concept", "topic"), [("", "sql"), (" ", "sql"), ("frames", "")]) + def test_blank_concept_or_topic_is_rejected_before_any_write( + self, scratch_db: Path, concept: str, topic: str + ) -> None: + """NOT NULL does not stop "" (council review 9, grok Y5): refuse at the boundary.""" + with pytest.raises(ToolError, match="non-empty"): + _get_tool("record_teachback")( + concept=concept, topic=topic, scores=VALID_SCORES, review_type="micro" + ) + assert _rows(scratch_db) == [] + + +class TestARowLands: + def test_a_valid_call_lands_exactly_one_row_through_the_real_schema( + self, scratch_db: Path, session_state + ) -> None: + session_state({"study_session_id": "study-42"}) + tool = _get_tool("record_teachback") + + result = tool( + concept="window frame", + topic="sql", + scores=VALID_SCORES, + review_type="structured", + angle="apply_network_analogy", + notes="explained ROWS vs RANGE unprompted", + ) + + assert result["recorded"] is True + assert result["total"] == sum(VALID_SCORES) + rows = _rows(scratch_db) + assert rows == [("window frame", "sql", None, 3, 3, 4, 3, 2)] + # The schema is the migrated one: its CHECK constraints are present. + conn = sqlite3.connect(scratch_db) + try: + ddl = conn.execute( + "SELECT sql FROM sqlite_master WHERE type='table' AND name='teach_back_scores'" + ).fetchone()[0] + finally: + conn.close() + assert "BETWEEN 1 AND 4" in ddl + + def test_the_study_session_is_reported_not_stored_as_the_rows_session_id( + self, scratch_db: Path, session_state + ) -> None: + """Finding N1 (S1-GREEN, corrected from the RED as first written). + + The RED assumed the live study session id would land in the row's + ``session_id``. The ownership layer forbids it: ``records.bind`` treats + ``session_id`` as a *native* harness session (it must exist in + ``sessions`` and be visible in scope) and links study sessions only for + ``parked_topics`` and ``study_notes`` -- the write failed and the tool + reported "not recorded". So the row is owned by scope, as the CLI's + rows are, and the study session id travels in the tool's reply. + """ + session_state({"study_session_id": "study-7"}) + result = _get_tool("record_teachback")( + concept="decorators", topic="python", scores=VALID_SCORES, review_type="micro" + ) + assert result["study_session_id"] == "study-7" + assert [row[2] for row in _rows(scratch_db)] == [None] + + def test_no_live_session_reports_none_and_still_records( + self, scratch_db: Path, session_state + ) -> None: + result = _get_tool("record_teachback")( + concept="decorators", topic="python", scores=VALID_SCORES, review_type="micro" + ) + assert result["study_session_id"] is None + assert len(_rows(scratch_db)) == 1 + + def test_a_repeated_call_is_two_rows_because_teachbacks_are_events( + self, scratch_db: Path, session_state + ) -> None: + tool = _get_tool("record_teachback") + for _ in range(2): + tool(concept="decorators", topic="python", scores=VALID_SCORES, review_type="micro") + assert len(_rows(scratch_db)) == 2 + + +class TestFailureIsLoud: + def test_no_connection_is_a_tool_error_not_a_silent_success( + self, scratch_db: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + import studyloop.history._connection as _conn + + monkeypatch.setattr(_conn, "_connect", lambda: None) + with pytest.raises(ToolError, match="not recorded"): + _get_tool("record_teachback")( + concept="decorators", topic="python", scores=VALID_SCORES, review_type="micro" + ) + assert _rows(scratch_db) == [] diff --git a/packages/studyloop/tests/test_verify_plan_integration_script.py b/packages/studyloop/tests/test_verify_plan_integration_script.py index 6e8b7ab71..8ecfd7236 100644 --- a/packages/studyloop/tests/test_verify_plan_integration_script.py +++ b/packages/studyloop/tests/test_verify_plan_integration_script.py @@ -464,11 +464,11 @@ def test_golden_sha_check_matches_the_committed_golden(self, script) -> None: assert code == 0, measured assert measured["sha256"] == script.GOLDEN_SHA256 - def test_inventory_check_reports_thirty_two_names_with_the_nine(self, script) -> None: + def test_inventory_check_reports_thirty_three_names_with_the_nine(self, script) -> None: code, measured = script.check_inventory_in_process(REPO_ROOT) assert code == 0, measured - assert measured["count"] == 32 - assert len(measured["names"]) == len(set(measured["names"])) == 32 + assert measured["count"] == 33 + assert len(measured["names"]) == len(set(measured["names"])) == 33 assert set(script.PLAN_TOOL_NAMES) <= set(measured["names"]) assert "record_plan_learning" in measured["names"] diff --git a/packages/studyloop/tests/test_web_session_ws.py b/packages/studyloop/tests/test_web_session_ws.py index 6d3196f0f..5e307e7f9 100644 --- a/packages/studyloop/tests/test_web_session_ws.py +++ b/packages/studyloop/tests/test_web_session_ws.py @@ -14,6 +14,9 @@ from __future__ import annotations +import asyncio +import gc +import logging import sys import time from pathlib import Path @@ -425,3 +428,89 @@ def test_ws_disconnect_releases_active_session( assert run_async(active.current()) is None assert stub.end_calls == 1 + + +# --------------------------------------------------------------------------- +# The drained pull future is retrieved on every exit +# --------------------------------------------------------------------------- + + +class _GatedStub(StubTransport): + """A transport whose stream drains only when the test says so. + + ``events()`` yields ``Started`` and then waits on an ``asyncio.Event`` + created in the server loop; setting it ends the generator, which is what + a real session's ``end()`` does when it pushes the queue sentinel. + """ + + def __init__(self) -> None: + super().__init__() + self.release: asyncio.Event | None = None + + async def events(self): # type: ignore[override] + self.release = asyncio.Event() + yield Started(agent="claude") + await self.release.wait() + + +class TestDrainedPullFuture: + def test_a_drain_that_lands_beside_a_takeover_is_retrieved( + self, + config: SessionConfig, + caplog: pytest.LogCaptureFixture, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """Regression: ``Task exception was never retrieved ... StopAsyncIteration``. + + The pump pulls each transport event as its own future. When a newer + socket takes the consumer slot at the same moment the stream drains, + the pump raised ``_SupersededError`` before reading the drained + future's ``StopAsyncIteration``, and its ``finally`` left a *done* + future neither cancelled nor read -- so asyncio logged the exception + from the future's finalizer. Seen live on 2026-09-23 when a PWA tab + reattached to a session whose transport had already ended. + """ + from studyloop.web.routes.session import _grace + + # The pump must wake because the pull completed, never because its + # supersede poll timed out first: that path cancels a still-pending + # pull and never exhibited the leak. + monkeypatch.setattr(_grace, "SUPERSEDE_POLL_S", 5.0) + stub = _GatedStub() + _install_active(stub, config) + + with ( + TestClient(create_app()) as client, + caplog.at_level(logging.ERROR, logger="asyncio"), + ): + portal = client.portal + assert portal is not None + with client.websocket_connect( + "/api/session/ws?study_session_id=study-1", + headers={"Origin": "http://127.0.0.1:8788"}, + ) as ws: + assert ws.receive_json() == {"type": "started", "agent": "claude"} + + def takeover_and_drain() -> None: + # The synchronous half of ``_grace.acquire_consumer``: a + # newer socket has claimed the slot ... + holder = _grace._attachment # pyright: ignore[reportPrivateUsage] + assert holder is not None + holder.superseded = True + # ... and the stream drains in the same loop turn. + assert stub.release is not None + stub.release.set() + + portal.call(takeover_and_drain) + frame = ws.receive_json() + assert frame["type"] == "attach_superseded" + + # A future whose exception was never read logs from its finalizer; + # force the finalizer so the assertion does not depend on refcount luck. + gc.collect() + leaked = [ + record.getMessage() + for record in caplog.records + if record.name == "asyncio" and "never retrieved" in record.getMessage() + ] + assert leaked == [], leaked[0] diff --git a/packages/studyloop/tests/test_writer_isolation.py b/packages/studyloop/tests/test_writer_isolation.py new file mode 100644 index 000000000..dfccf1860 --- /dev/null +++ b/packages/studyloop/tests/test_writer_isolation.py @@ -0,0 +1,131 @@ +"""Writer isolation (learning tier, item 1, plan §5 "Isolation"). + +Every ``W_auto`` writer is driven in a **child process** whose ``HOME`` and +``XDG_*`` point at a decoy tree while the four StudyLoop redirects point at a +sandbox. If any writer resolved a path through the home directory instead of +the redirect, a test run would reach the learner's real database or session +files -- the exact failure ``conftest.py`` documents from 2026-09-05. The +child writes through the real MCP tool functions, then the parent asserts: + +* the decoy home tree holds no new file (including Markdown); +* each writer's row or line landed inside the sandbox. + +Plan §5 says this test is a guard if it already passes on ``main`` and RED +only if it fails; it passed on first run, so it is the guard. +""" + +from __future__ import annotations + +import json +import os +import sqlite3 +import subprocess +import sys +import textwrap +from typing import TYPE_CHECKING + +import pytest + +if TYPE_CHECKING: + from pathlib import Path + +pytest.importorskip("mcp") + +_CHILD = textwrap.dedent( + """ + import json, os, sys + from studyloop.mcp.server import mcp + tools = mcp._tool_manager._tools + + def call(name, **kw): + return tools[name].fn(**kw) + + out = {} + out["log_topic"] = call("log_topic", topic="window frame", status="learning", note="iso") + out["log_struggle"] = call("log_struggle", question="why ROWS not RANGE", topic_tag="sql") + out["record_teachback"] = call( + "record_teachback", concept="window frame", topic="sql", + scores=[3, 3, 4, 3, 2], review_type="micro", + ) + plan = call( + "create_study_plan", title="Isolation plan", + answers={"why": "prove the writers stay in the sandbox", "success": ["one row each"], + "topics": ["sql"], "milestones": [{"title": "Frames"}]}, + plan_id="isolation-plan", + ) + out["record_plan_learning"] = call( + "record_plan_learning", plan_id="isolation-plan", title="Frames are ROWS or RANGE", + body="isolation body", + ) + print(json.dumps(out, default=str)) + """ +) + + +def _tree(root: Path) -> set[str]: + return {str(p.relative_to(root)) for p in root.rglob("*") if p.is_file()} + + +def test_every_w_auto_writer_stays_inside_the_sandbox(tmp_path: Path) -> None: + decoy_home = tmp_path / "decoy-home" + sandbox = tmp_path / "sandbox" + for d in (decoy_home, sandbox / "state", sandbox / "session", sandbox / "plans"): + d.mkdir(parents=True) + # A decoy config dir that looks like a real one, so a writer that resolves + # through HOME has somewhere plausible to land. + (decoy_home / ".config" / "studyloop").mkdir(parents=True) + (decoy_home / ".local" / "share" / "studyloop").mkdir(parents=True) + before = _tree(decoy_home) + + env = { + k: v + for k, v in os.environ.items() + if not k.startswith(("STUDYLOOP_", "XDG_")) and k != "HOME" + } + env.update( + { + "HOME": str(decoy_home), + "XDG_CONFIG_HOME": str(decoy_home / ".config"), + "XDG_DATA_HOME": str(decoy_home / ".local" / "share"), + "STUDYLOOP_DB": str(sandbox / "sessions.db"), + "STUDYLOOP_STATE_DIR": str(sandbox / "state"), + "STUDYLOOP_SESSION_DIR": str(sandbox / "session"), + "STUDYLOOP_PLANS_DIR": str(sandbox / "plans"), + "STUDYLOOP_CONFIG": str(sandbox / "config.yaml"), + } + ) + proc = subprocess.run( + [sys.executable, "-c", _CHILD], + env=env, + capture_output=True, + text=True, + timeout=120, + check=False, + ) + assert proc.returncode == 0, proc.stderr[-2000:] + out = json.loads(proc.stdout.strip().splitlines()[-1]) + + # Nothing escaped to the decoy home -- not a database, not a Markdown file. + escaped = _tree(decoy_home) - before + assert escaped == set(), f"writers reached the home tree: {sorted(escaped)}" + + # Each writer landed inside the sandbox -- rows counted, not files stat'ed + # (council review 9, astra Y4). + assert out["record_teachback"]["recorded"] is True + assert out["record_plan_learning"]["created"] is True + assert out["log_struggle"]["status"] == "logged" + topics = sandbox / "session" / "session-topics.md" + assert topics.is_file() and "window frame" in topics.read_text(encoding="utf-8"), ( + "log_topic wrote no session line" + ) + conn = sqlite3.connect(sandbox / "sessions.db") + try: + teachbacks = conn.execute("SELECT COUNT(*) FROM teach_back_scores").fetchone()[0] + parked = conn.execute("SELECT COUNT(*) FROM parked_topics").fetchone()[0] + finally: + conn.close() + assert teachbacks == 1, f"record_teachback rows in the sandbox: {teachbacks}" + assert parked == 1, f"log_struggle rows in the sandbox: {parked}" + plan_docs = list((sandbox / "plans").rglob("*.md")) + assert plan_docs, "record_plan_learning wrote no plan document in the sandbox" + assert any("Frames are ROWS or RANGE" in p.read_text(encoding="utf-8") for p in plan_docs) diff --git a/scripts/update-agent-manifest.py b/scripts/update-agent-manifest.py index 60bcea30a..7aa71e5b5 100644 --- a/scripts/update-agent-manifest.py +++ b/scripts/update-agent-manifest.py @@ -42,6 +42,7 @@ "shared/session-protocol.md", "shared/session-db-mandate.md", "shared/teach-back-protocol.md", + "shared/recording-protocol.md", "shared/network-bridges.md", "shared/break-science.md", "shared/wind-down-protocol.md", diff --git a/scripts/verify/plan_integration.py b/scripts/verify/plan_integration.py index 7de99fc30..3e756ca1f 100644 --- a/scripts/verify/plan_integration.py +++ b/scripts/verify/plan_integration.py @@ -62,8 +62,9 @@ "ec451ce8857c8a72e398e3054e3c060cd3b5b13ecb29e29cba3822dc192503c0" # pragma: allowlist secret ) -#: The production inventory: 23 original tools + the nine plan tools (review 3, F13). -PRODUCTION_TOOL_COUNT = 32 +#: The production inventory: 23 original tools + the nine plan tools (review 3, F13) +#: + record_teachback (learning tier item 1, 2026-09-23). +PRODUCTION_TOOL_COUNT = 33 CORE_TOOLS = frozenset({"list_courses", "get_study_backlog", "end_session"}) #: Protected test files: byte-identical to their base since the programme began. @@ -148,7 +149,7 @@ def check_golden_sha(repo_root: Path) -> tuple[int, dict[str, Any]]: def check_inventory_in_process(repo_root: Path) -> tuple[int, dict[str, Any]]: - """The in-process twin of the stdio inventory: exactly 32 unique names, + """The in-process twin of the stdio inventory: exactly 33 unique names, the nine plan tools, ``record_plan_learning`` and the core names.""" _ = repo_root from studyloop.mcp.server import mcp