feat(learning-tier): item 1 — the mentor writes (S1-0 receipt) - #41
Merged
Merged
Conversation
…s from source, writer sets frozen Item 1 (#38) stage S1-0. Records, with file:line on main c1a28de, how each of the six harnesses grants tool use to the mentor today and freezes W_auto and W_srs. Written to the owner decisions on PR #36: W_auto is named in every definition and prompt-per-call everywhere, no new pre-approval on any harness; kiro keeps its log_topic entry as the one recorded asymmetry; Claude names none of the studyloop MCP tools today so naming is what makes the writers reachable; opencode wildcard recorded as known and out of scope. Also pins the record_teachback MCP contract to the CLI validator it must mirror.
…erywhere, one recording protocol Item 1 (#38) stage S1-RED, per plan §5 and the S1-0 receipt. 22 red, each for its stated reason on main c1a28de: - test_mcp_teachback.py (new, 13 red): the tool is absent from the MCP registry. Pins the contract: validates exactly as cli/_teachback.py (five ints 1-4, review_type enum), lands one row through the real migrated schema (CHECK present), no row on failure, session_id bound from session state and refused from the caller, a repeated call is two rows, a missing connection is a ToolError. - test_adapter_parity.py (2 red, 4 guards green): no definition names the four W_auto writers; Claude tools: line names none. Guards pin decision 2 - kiro allowedTools keeps exactly log_topic, Claude settings.json has no permissions block, opencode block as recorded. - test_docs_harness_tier_contract.py (7 red): agents/shared/recording-protocol.md absent, so no trigger table, no references, no manifest hash; persona.md:32 still routes "record progress" to tutor-checkpoint. test_writer_isolation.py is deferred to GREEN (it needs record_teachback to exist to run each W_auto writer; plan §5 says it is a guard, not RED, if it passes).
…ares Item 1 (#38) S1-GREEN, part 1. The mentor had no MCP writer for a teach-back score; the only writer was the CLI. record_teachback now validates through one shared implementation (history.teachback.coerce_scores / coerce_review_type, extracted from cli/_teachback.py with the CLI messages kept verbatim; the CLI delegates to it), lands one row through history.record_teachback, raises ToolError on any rejection or an unavailable database, and treats a repeated call as a second row. Finding N1, corrected from the RED as first written: the RED assumed the live study session id would land in teach_back_scores.session_id. The ownership layer forbids it - records.bind treats session_id as a native harness session that must exist in `sessions` and be visible in scope, and links study sessions only for parked_topics and study_notes - so the write failed and the tool reported "not recorded". The row is therefore owned by scope exactly as the CLI rows are, and the study session id travels in the reply. The two RED expectations are rewritten to that, with the finding stated in the test. 13/13 in test_mcp_teachback; CLI, history and the other MCP suites 100/100.
…o persona stops routing to tutor-checkpoint Item 1 (#38) S1-GREEN, part 2. agents/shared/recording-protocol.md is the one instruction that says WHEN the mentor writes: a fenced YAML trigger table (trigger, when, writer, required_ids, consent) naming exactly the four W_auto writers, one line of prose per trigger, and the rule that the SRS mutators never fire from it. Every mentor definition now references it with an identical sentence (codex and pi AGENTS.md, opencode and claude mentors; grok reads the codex file). Claude tools: line names the studyloop readers and the four writers - naming, not approval: no permissions block is added, per owner decision 2. Kiro persona.md no longer routes "record progress" to uv run tutor-checkpoint (a different tool, left to its own skill); it names the MCP writers and keeps the CLI as the human path. teach-back-protocol.md Recording section puts the MCP tool first with the consent rule; contributing.md tells a harness author what the parity tests will pin. agents/manifest.json regenerated: hashes change for exactly the six touched files. test_writer_isolation.py added as the plan §5 guard (passes first run, so not RED): a child process with decoy HOME/XDG runs all four writers and nothing lands outside the sandbox. S1-RED f439ab8 flips: 84/84 across the RED files, install contracts, isolation and CLI.
Four deliberate exact-count pins guard the MCP registry against an accidental or duplicate registration: test_mcp_plan_tools, test_mcp_stdio_smoke, the verify script and its test, plus the MCP README table and its count sentence (docs contract). Each names the new tool in its arithmetic comment rather than just bumping the number. Found by the full-suite set comparison against the committed environmental ids.
…ilt from personas/*.md, not the installed definitions Council review 9, coordinator finding while verifying grok Y2: studyloop study renders agents/shared/personas/<mode>.md through build_canonical_persona and hands THAT to every adapter (Kiro prompt, Claude flag file, session-dir AGENTS.md for Codex/pi/Grok). The six installed definitions GREEN named the writers in are what a learner gets when opening the harness directly. study.md told the mentor to show the learner a studyloop topic command and never to write - the defect in its live form - and named no writer. study.md gains a Recording section (four triggers, the protocol, the unsuccessful-write line) and lists the writers under Progress & Review; co-study.md gets the same four with its student-drives rule. test_adapter_parity pins the BUILT persona for study and co-study, not a file; the docs contract adds both live personas to DEFINITIONS. Also landed here, same files: grok Y2 (pin that Grok projects the Codex definition and that agents/grok/ does not exist), astra Y2 (kiro guard also refuses wildcard grants; opencode block parsed and compared whole, not substring-matched). S1-0 receipt §5 records both corrections as an addendum.
…failure honestly Council review 9. grok Y5: NOT NULL does not stop "", so a blank concept or topic became a row nothing can find again - refused at the boundary before any write, three parametrised tests. grok Y1 (partially accepted): the writer returns False for no-database, lock/timeout and refused-row alike and does not say which, so the ToolError now states the three rather than claiming "unavailable". Splitting them properly needs the writer to distinguish - the same collapse its CLI caller inherited - and is not done here. grok claimed ScopeError was also swallowed; verified false: record_teachback catches only IntegrityError and OperationalError, a ScopeError propagates.
…licit unsuccessful-write line Council review 9. qwen R2 (partially accepted, not as a blocker): the teach_back_agreed when-clause now names what is observable - five scores proposed in one sentence, the learner next reply accepts them - instead of "the learner agreed them". astra Y5: a section says what to do when the write does not happen (refusal, declined prompt, database unavailable): one line, continue, never retry silently, never claim a record that did not land. Manifest hash and secrets baseline regenerated for the one changed file.
Council review 9, astra Y4: "each writer landed" was asserted by a file existing. Now the sandbox sessions.db is opened and teach_back_scores and parked_topics are counted (one row each), and the topics file must carry the logged concept.
…ipts, arbitration (GATE ACCEPT at 616fd1e) Three seats (openai.gpt-6-astra ACCEPT-WITH-CORRECTIONS, grok-4.6 ACCEPT, qwen3-coder REJECT) on the item-1 tree 3c175f7. Every finding verified against the tree before action; nine landed across ad6d1f9, 019624c, 2a4d4d1, 616fd1e; four refuted with evidence (qwen R1, qwen Y, astra Y1, grok Y4 mechanism); qwen REJECT not sustained. The coordinator own finding - the live persona is built from personas/*.md and named no writer - is recorded as a gap none of the seats saw. Seat copies: git diff -w against the originals is empty; originals sha256 prefixes recorded in the arbitration. grok (e) ten-item list adopted as the S1-SIM checklist.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical contract and database-error-handling findings remain unresolved, along with persona and test-contract corrections.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 3
Open (7)
What changed in this PR
Adds the learning-tier record_teachback MCP writer, shared validation, and mentor recording guidance across supported harnesses.
Changes:
- Adds MCP persistence, validation reuse, and writer isolation coverage.
- Updates recording protocols, personas, tool inventories, and manifests.
- Adds capability-lock and council-review documentation.
| File | Summary |
|---|---|
scripts/verify/plan_integration.py |
Updates the MCP inventory pin; the canonical OpenSpec inventory remains stale (nit). |
scripts/update-agent-manifest.py |
Tracks the shared recording protocol. |
packages/studyloop/tests/test_writer_isolation.py |
Adds sandbox isolation coverage for writers. |
packages/studyloop/tests/test_verify_plan_integration_script.py |
Updates inventory verification assertions. |
packages/studyloop/tests/test_mcp_teachback.py |
Tests teach-back behavior; transport-level coverage and shared database fixture reuse remain needed (moderate, critical). |
packages/studyloop/tests/test_mcp_stdio_smoke.py |
Updates stdio inventory coverage. |
packages/studyloop/tests/test_mcp_plan_tools.py |
Updates tool inventory assertions. |
packages/studyloop/tests/test_docs_harness_tier_contract.py |
Validates protocol contracts; the required when key remains unenforced (moderate). |
packages/studyloop/tests/test_adapter_parity.py |
Pins writer names and harness grants. |
packages/studyloop/src/studyloop/mcp/tools.py |
Implements record_teachback; operational database failures can still leak raw SQLite errors (critical). |
packages/studyloop/src/studyloop/history/teachback.py |
Provides shared teach-back validation. |
packages/studyloop/src/studyloop/cli/_teachback.py |
Reuses shared validation for the CLI. |
docs/contributing.md |
Documents harness parity requirements. |
docs/architecture/learning-tier/receipts/s1-0-capability-lock.md |
Records capability-lock decisions. |
docs/architecture/learning-tier/council/review9/seat-qwen3-coder.md |
Records council review evidence. |
docs/architecture/learning-tier/council/review9/seat-openai.gpt-6-astra.md |
Records council review evidence. |
docs/architecture/learning-tier/council/review9/seat-grok-4.6.md |
Records council review evidence. |
docs/architecture/learning-tier/council/review9/manifest.json |
Records council run metadata. |
docs/architecture/learning-tier/council/review-9-arbitration-2026-09-23.md |
Records arbitration and gate decisions. |
docs/architecture/learning-tier/council/brief-review9-2026-09-23.md |
Records the review brief and scope. |
agents/shared/teach-back-protocol.md |
Routes teach-back recording through MCP. |
agents/shared/recording-protocol.md |
Defines writer triggers; its struggle condition conflicts with the prose requirement (moderate). |
agents/shared/personas/study.md |
Adds study-mode recording guidance; an unconditional CLI instruction conflicts with MCP writing (moderate). |
agents/shared/personas/co-study.md |
Adds co-study recording guidance; existing CLI instructions conflict with MCP writing (moderate). |
agents/pi/AGENTS.md |
References the recording protocol. |
agents/opencode/study-mentor.md |
References the recording protocol. |
agents/mcp/README.md |
Documents the 33-tool inventory; the active OpenSpec contract still requires 32 tools (critical). |
agents/manifest.json |
Updates agent manifest hashes. |
agents/kiro/study-mentor/persona.md |
Replaces obsolete progress routing. |
agents/codex/AGENTS.md |
References the recording protocol. |
agents/claude/socratic-mentor.md |
Exposes the relevant readers and writers. |
.secrets.baseline |
Refreshes generated secret-scan metadata. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| ## 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 |
Comment on lines
+916
to
+931
| 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`" | ||
| ) |
Comment on lines
+68
to
+73
| @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 |
Comment on lines
+34
to
+38
| 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. |
Comment on lines
36
to
+53
| **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. |
Comment on lines
+191
to
+193
| REQUIRED_KEYS: ClassVar[frozenset[str]] = frozenset( | ||
| {"trigger", "writer", "required_ids", "consent"} | ||
| ) |
Comment on lines
+46
to
+51
| 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 | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #38 — lands S1-0, S1-RED, S1-GREEN and council review 9. Does not close #38: S1-SIM (owner-gated harness logins) and the noticing episode stay open there, and decision 3 defines the noticing episode as the first real session after S1-GREEN reaches
main, so holding this reviewed, CI-green tree on a branch until then would block the very stage it gates. The 2026-09-23 branch census (~244 finished commits that never landed) is the reason not to wait.What this delivers
The mentor writes to the learning tier. Before this branch it only read: no MCP teach-back writer existed, the Kiro persona routed "record progress" to a different tool, and no harness had a trigger table — zero writer calls in 143,973 historical messages. Plan:
docs/architecture/learning-tier/plan-2026-09-19.md§3; owner decisions 1–5 in §7 (PR #36).Branch cut from
main@c1a28de1(decision 1). Item 2 is not here (#39).Stages
fd70817c.docs/architecture/learning-tier/receipts/s1-0-capability-lock.md: grant mechanism per harness from source with file:line;W_auto/W_srsfrozen; therecord_teachbackcontract pinned to the CLI validator. Rule this branch:W_autonamed in every definition, prompt-per-call everywhere, no new pre-approval on any harness (decision 2).f439ab84. 22 red, each for its stated reason:test_mcp_teachback.py(13, tool absent),test_adapter_parity.py(2 red + 4 guards pinning decision 2),test_docs_harness_tier_contract.py(7: protocol absent,persona.md:32still namestutor-checkpoint).7354ea3a+0a22aabe+3c175f78.record_teachbackMCP tool validating through one shared implementation extracted fromcli/_teachback.py(the CLI delegates to it; its messages are unchanged).ToolErroron any rejection or an unavailable database; a repeated call is two rows.agents/shared/recording-protocol.md: fenced YAML trigger table (trigger,when,writer,required_ids,consent) naming exactly the four writers; SRS mutators never fire from it. Referenced by an identical sentence in every mentor definition; hashed inagents/manifest.json(six touched files changed hash, nothing else).tools:line names the readers and the four writers — naming, not approval; no permissions block anywhere. Kiro persona no longer routes totutor-checkpoint. Teach-back protocol's Recording section puts MCP first with the consent rule.docs/contributing.mdtells a harness author what the parity tests pin.test_writer_isolation.py: a child process with a decoyHOME/XDGruns all four writers; nothing lands outside the sandbox. Passed on first run, so it is the plan §5 guard, not a RED.5670f941. Seats: astra ACCEPT-WITH-CORRECTIONS, grok ACCEPT, qwen REJECT (not sustained: its two 🔴 are refuted against the tree). Nine findings landed one concern per commit (ad6d1f9d,019624c3,2a4d4d1d,616fd1ed), four refuted with evidence, arbitration GATE ACCEPT at616fd1ed. Coordinator's own finding, found while verifying a seat's mechanism: every live session is built fromagents/shared/personas/<mode>.md, not the installed definitions —study.mdnamed no writer and told the mentor to show the learner astudyloop topiccommand. Now named and pinned on the BUILT persona (test_the_built_live_persona_names_each_writer); receipt §5 records the correction.tools:-line reachability and installed-path resolution ofagents/shared/…deciding{passed}. Owner-gated: harness logins.main).Finding N1 (corrected from the RED as first written)
The RED assumed the live study session id would land in
teach_back_scores.session_id. The ownership layer forbids it:records.bindtreatssession_idas a native harness session that must exist insessionsand be visible in scope, and links study sessions only forparked_topicsandstudy_notes— the write failed and the tool reported "not recorded". So the row is owned by scope exactly as the CLI's rows are, and the study session id travels in the tool's reply. Stated in the test and the GREEN commit, not erased.Verification
Scoped: RED files + install contracts + isolation + CLI 84/84; MCP suites 100/100; inventory suites 180/180; stdio smoke 2/2. ruff, format, pyright clean on every touched file; mkdocs strict clean;
.secrets.baselineupdated by a whole-repo scan (onlyagents/manifest.jsonentries changed, 72 → 72 files).Full
pytest(unit, 15:43): 7340 passed, 35 failed, 14 errors. Failing ids minus the committed environmental set (full-suite-control-item4-2026-09-18.md) = the four inventory pins, fixed in3c175f78, plustest_concatenated_remote_dump_with_existing_archive— the pre-existingsqlite3 -bail60 s timeout inagent-session-toolsrecorded on #35, which fails identically on cleanmain.Claim rule
The final PR body will state exactly "pipe open on {passed}; plumbing proven for all six definitions; noticing observed once", listing which harnesses passed and the skip reason for each of the rest. No skip is counted as a pass.
Gates before merge
CI green on the reviewed tree; the S1-SIM stage above.