Follow-on bookkeeping: F8 pins, verify base, closeout record, item-6 proposals, T5.1 - #24
Merged
Merged
Conversation
Review 6 accepted two behaviours without a dedicated pin and recorded them as cheap follow-ons: `plan repair` exits 0 on a non-active unready plan (the behaviour was pinned only implicitly by the ready-plan sibling), and the duplicate-record short-circuit in `_revise` treats a mission edit as a change (exercised by item 3b's tests, never pinned on its own). test_plan_repair_nonactive_unready_is_noop_with_pointer: a plain draft is not a husk — nothing refuses its writes — so `plan repair` has nothing to unblock: exit 0, the "is draft, so nothing blocks it" sentence and the `studyloop plan architect` pointer, no launch (the launch chain is patched and records zero calls), no write (documents byte-equal before and after), status unchanged, and NOT the ready plan's "Nothing to repair" sentence. test_duplicate_learning_record_with_mission_revision_still_saves_once: a retried record beside a sharpened `why` is one save — not zero (the `why` would be dropped while reporting "already recorded") and not two. Discrimination proved by mutation, sources restored byte-identical: with `if s.status != "active":` mutated to fall through, the repair pin fails and the ready-plan sibling stays green; with `not mission_updates` dropped from `duplicate_record_only`, the record pin fails and the field-change sibling stays green. Both modules 78/78; ruff/pyright clean.
…e the check by role PROTECTED_EARLY_BASE was 3a4f6b01, the programme's first RED commit. The history consolidation rewrote the early commits: that sha survives only as an unreachable object in this one clone (`git fetch origin 3a4f6b01` finds no such ref; `git for-each-ref --contains` lists nothing), so the `protected-files-3a4f6b01` check — `git diff --quiet 3a4f6b01 -- <3 files>` — would fail on every fresh checkout while passing here. Found while re-verifying the issue-closeout draft's shas against main before posting. The base moves to d7f568b, the same commit on main ("RED — pin the two plan bugs issue #7 named as must-fix-first", found by exact subject); the three protected files are byte-identical between the two (`git diff --stat` empty) and the check exits 0 against the new base. The check is renamed `protected-files-early-base` to match `protected-files-late-base` (0be141b did the same when the late base moved): a base is a moving pin by design, and the name should not have to move with it. The registry test drops the sha from the check name and pins the property the old base lacked: test_protected_file_bases_are_reachable_from_main asserts both bases are ancestors of main (proved discriminating: with the old sha restored it fails "3a4f6b01 is not an ancestor of main"). 27/27; ruff/pyright clean.
…state; stage-9 header corrected Item 7 step 3 ran on 2026-09-18: the closeout draft's per-issue tables were posted as status comments on #8–#15 after re-verification against main at a5b9f90 — every T: node id against the collected suite, every C: sha against main. Seven pre-consolidation shas were rewritten and replaced in the comments by their main equivalents (found by exact subject), one test had moved files, and four rows the follow-on programme had overtaken (#11's Markdown-only mission, #13's "partly", #14/#15/#7's brain-dump and cancellation gaps, #10's rubric state) were restated as they stand today. #8, #9, #11, #12, #13, #14 closed as completed; #10 and #15 stay open on rubric row 3b (item 5); #7 — auto-closed at PR #20's merge by the body's "Closes the two bugs" phrase — reopened with the parent mapping so it closes after its children, as the handover intends. The draft's status paragraph records all of this above the unchanged original text. tasks.md: the two F8 pins are landed (5d9c480); T7.1 carries the per-step state of HANDOFF §3 item 7 — steps 1–3 done, 4–7 open, three of them owner-only (tag, support ticket, D-J token revocation). The stage-9 script's header said agents cannot delete remote refs by platform policy. Wrong, and now disproved on 2026-09-17 and 2026-09-18: the harness allows a remote branch deletion that names its branch literally; the "Default" ruleset is what refuses it, for everyone. The header says so.
…; tick T6.4 Two proposals under docs/architecture/plan-integration/proposals/, no code (HANDOFF §3 item 6). Each quotes the owner's decision verbatim, states what the engine and store do today from the tree at a5b9f90 rather than from the design text, then the proposal, its constraints, what is out of scope, and the acceptance the issue will carry. 2026-09-16-context-derived-plan-bias.md (D-D): replace the single PLAN_RELATED_BIAS = 12 with a derived, bounded bias from three deterministic inputs — today's plan relevance (5a), prerequisite order from list_dependencies' `relation_type == "prerequisite"` edges, which weak_links_for_topic already reads (5b), and per-item energy demand, taking item 5's definition rather than inventing a second (5c). No model call in ranking (unauditable; defeats D-16); an absent edge is no signal; the golden and the three rule-5 pins stay green; the concept-store read is budgeted and measured on a real sessions.db before it ships, as item 4's preview read was. 2026-09-16-overdue-nudge-and-retire.md (D-E): a due row is a study_progress row whose last_seen has reached a REVIEW_INTERVALS step; there is no next_review column and no "not due" state, so a row stays a candidate until studied again, and at 100 + min(days_ago, 30) an unrelated overdue item silently overtakes the +12 plan bias after ~12 days. Proposed: an age-aware nudge line on an unrelated due candidate past a threshold derived from the constants (so it moves with D-D), and learner-issued retire/snooze states on the row through the seam (CLI verb, Today control, one MCP tool — the mirror of record_topic_progress(confidence="resolved"), which today resolves only parked topics). History kept; never inferred from age; excluded from every consumer of spaced_repetition_due (now, recap, `studyloop review`, the plan evaluation); flashcards' SM-2 store is a separate ticket. mkdocs --strict exit 0; the six docs guard modules 211/211 with the new files in place. tasks.md T6.4 ticked; the two issues are item 7 step 4. .gitignore: docs/architecture/plan-integration/* is an allowlist (council/, receipts/, the archify spec, top-level .md); proposals/ was never on it, so the first `git add` was refused. Added in the same shape, Markdown only.
…hree amendments Item 5 (D-F) was designed on 2026-09-16 before its RED. Read against learning/decision.py at 7208eb6, three of its sentences do not fit the tree; each is amended under §5 with the source it rests on, so T5.2's RED is written against a design the code can carry. 1. "recovered / gentle review -> low" names a row _struggle_candidates never emits: it selects only confidence in ("struggling", "learning") or last_teachback_score < 14. The low-demand class is the `learning` row; fresh `struggling` (last_seen <= 14 days) is high, old `struggling` or a weak-teach-back-only row is medium. Demand is derived once in the collector and carried in candidate metadata. 2. "listed in energy_deferred" cannot hold a repair: DeferredMilestone has a mandatory milestone_index and all three renderers (cli/_now.py, learning/recap.py, today-panel.js deferredNotes) print `milestone {index + 1} "{title}"`. A deferred repair gets its own frozen DeferredRepair in a new additive key, energy_deferred_repairs, and each renderer gains one line for it. 3. "opens the existing body-double session route (web/routes/body_double.py)" names the read-only focus reader. The session door is `studyloop study "<topic>" --mode co-study` on the CLI and the Body Double view's session start on the Web; _evidence_command has no conversation branch and would fall through to `studyloop progress … -c learning` (a write), so the body-double candidate sets its evidence_command explicitly. tasks.md T5.1 ticked with the receipt. Full suite on this branch: 30 failed / 5120 passed / 14 errors, the 44 failed+errored ids byte-identical to the item-4 control's committed environmental set (run - control = empty).
There was a problem hiding this comment.
🟡 Changes recommended
Moderate unresolved findings remain in CI base resolution and the overdue-retire proposal behavior.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR records plan-integration follow-up work, including verification updates, regression tests, closeout records, design amendments, and future proposals.
Changes:
- Repointed protected-file verification to a reachable base and added coverage.
- Added regression tests for repair and duplicate-record persistence.
- Recorded task status, closeout details, design amendments, and proposals.
File summaries
| File | Reviewed change |
|---|---|
scripts/verify/plan_integration.py |
Updates the protected early-base pin and check name. |
scripts/maintenance/stage9-owner-remote-cleanup.sh |
Corrects cleanup guidance. |
packages/studyloop/tests/test_verify_plan_integration_script.py |
Adds protected-base reachability coverage. |
packages/studyloop/tests/test_plan_application_mutations.py |
Tests mission revision and duplicate-record persistence. |
packages/studyloop/tests/test_cli_plan_seam.py |
Tests non-active repair no-op behavior. |
openspec/changes/plan-integration-followons/tasks.md |
Records follow-on task status. |
openspec/changes/plan-integration-followons/design.md |
Records T5.1 design amendments. |
docs/architecture/plan-integration/receipts/issue-closeout-draft-2026-09-16.md |
Records issue closeout status. |
docs/architecture/plan-integration/proposals/2026-09-16-overdue-nudge-and-retire.md |
Adds the D-E proposal. |
docs/architecture/plan-integration/proposals/2026-09-16-context-derived-plan-bias.md |
Adds the D-D proposal. |
.gitignore |
Allows proposal documents to be tracked. |
Review details
Suppressed comments (3)
docs/architecture/plan-integration/proposals/2026-09-16-overdue-nudge-and-retire.md:66
record_study_progressis the flashcard scheduler tool (course,card_hash,correct), while the MCPlog_topicpath writesstudy_progressandrecord_topic_progressoperates on parked backlog rows. The proposal explicitly excludes flashcards below, so namingrecord_study_progresshere points the future retire/snooze API at the wrong store; identify the intended MCP surface.
Exposed the same way the plan lifecycle is: a CLI verb (`studyloop review retire|snooze`), a Today-card
control on the due item, and one MCP tool beside `record_study_progress` — the mirror of
`record_topic_progress(confidence="resolved")` for cards that the decision asks for. The store writes
docs/architecture/plan-integration/proposals/2026-09-16-overdue-nudge-and-retire.md:114
- The schema inventory is not the current schema: migrations v10, v13, v22, and v27 add teach-back, concept, provenance, and source-session columns after the v9 columns listed here. Because the acceptance explicitly requires migration and clean-rebuild coverage, label this as the v9 baseline or include the later columns.
`agent_session_tools/migrations.py` (`study_progress` columns: `id, topic, concept, confidence,
first_seen, last_seen, session_count, notes, created_at, updated_at`).
openspec/changes/plan-integration-followons/design.md:242
- These amendments leave the earlier §5 bullets contradictory: lines 225–226 still define a
recovered / gentle reviewdemand class, 227–229 still put deferred repairs inenergy_deferred, and 230–234 still identifyweb/routes/body_double.pyas the session door. This paragraph asserts each of those is false, so update or remove the old bullets instead of leaving two conflicting design contracts.
**T5.1 review against the code (2026-09-18, tree `7208eb67`) — three amendments, each from reading
`learning/decision.py`, not the text above:**
- Files reviewed: 10/11 changed files
- Comments generated: 6
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+61
to
+67
| | **retire** | "I am done with this concept for now" | a `retired_at` timestamp on the row (or a `confidence` value the due predicate excludes — decided at RED with the migration); the row is no longer due, keeps its history, and disappears from every consumer of `spaced_repetition_due` — `now` (CLI, Today, MCP `get_next_action`), recap, `studyloop review`, and the plan evaluation's due count | studying it again (`record_progress`) clears the state | | ||
| | **snooze** | "not this week" | a `snoozed_until` date; the row is not due before it and returns to the normal intervals after | expiry, or an explicit un-snooze | | ||
|
|
||
| Exposed the same way the plan lifecycle is: a CLI verb (`studyloop review retire|snooze`), a Today-card | ||
| control on the due item, and one MCP tool beside `record_study_progress` — the mirror of | ||
| `record_topic_progress(confidence="resolved")` for cards that the decision asks for. The store writes | ||
| one row; retire is never inferred from age (the decision says *let the learner* retire it). |
|
|
||
| for base in (script.PROTECTED_EARLY_BASE, script.PROTECTED_LATE_BASE): | ||
| result = subprocess.run( | ||
| ["git", "merge-base", "--is-ancestor", base, "main"], |
Comment on lines
+27
to
+29
| the same intervals, and the only thing that moves `last_seen` is studying it again | ||
| (`record_progress`). A row therefore stays due — and stays a `now` candidate — until it is studied | ||
| or deleted. |
Comment on lines
+46
to
+49
| When an eligible due candidate is **unrelated to every matchable plan** and has been due long enough | ||
| to have overtaken the plan bias — i.e. `days_ago ≥ threshold`, with the threshold **derived from the | ||
| constants**, not a second number: the smallest `days_ago` at which `min(days_ago, 30) ≥ PLAN_RELATED_BIAS` | ||
| plus the plan-related candidate's own age — the candidate carries a `nudge` line that says so in plain |
| repair needs its own `DeferredRepair` in a new additive `energy_deferred_repairs` key because `DeferredMilestone` | ||
| and its three renderers are milestone-shaped; the body-double door is `studyloop study … --mode co-study` / | ||
| the Body Double view's session start, not the read-only `body_double.py` focus route, so the candidate sets | ||
| its `evidence_command` explicitly. T5.2's RED names hold; a sixth test pins the new key's rendering.) |
Comment on lines
+70
to
+76
| # Moved 3a4f6b01 -> d7f568bf on 2026-09-18: the history consolidation rewrote | ||
| # the programme's early commits, and 3a4f6b01 survived only as an unreachable | ||
| # object in one clone (`git fetch origin 3a4f6b01` finds no such ref), so the | ||
| # check would fail on any fresh checkout. d7f568bf is the same commit ("RED -- | ||
| # pin the two plan bugs issue #7 named as must-fix-first") on main; the three | ||
| # protected files are byte-identical between the two (git diff --stat empty). | ||
| PROTECTED_EARLY_BASE = "d7f568bf" |
… (audit red) CI's `audit` and `audit-full` jobs went red on PR #24 (run 35380095596) while the same commands were green on main at a5b9f90 an hour earlier: pip-audit now reports two advisories against anyio 4.12.1, both fixed in 4.14.2. Nothing on this branch touches a dependency; the advisories were published in between. Reproduced locally with the workflow's exact command (`uv export … | pip-audit --strict --no-deps --disable-pip`). anyio is transitive (no workspace member pins it; six lock dependents, none with a specifier), so `uv lock --upgrade-package anyio` is the whole change: anyio 4.15.1 and its own typing-extensions 4.15.0 -> 4.16.0. Both audit commands now report "No known vulnerabilities found"; the modules that exercise anyio at runtime (MCP stdio smoke, session start on both transports, web plan routes, the combined journey, LAN auth) pass 306/306 on the synced lock.
…ed (CI shallow checkout) Run 35380095596 failed the 3.12 and 3.13 matrix on PR #24 while every other job passed and main was green an hour earlier. The cause is the new test_protected_file_bases_are_reachable_from_main from 34eb537: it runs `git merge-base --is-ancestor <base> main`, and CI's actions/checkout is a depth-1 clone of the PR ref with no `main` at all — there the base is not even an object ("fatal: Not a valid object name d7f568b"). Reproduced locally in a `--depth 1` clone of the branch: same failure. The property the test pins — both protected-files bases reachable on origin — cannot be judged in that checkout, so the test now skips with the reason when the repository is shallow (`rev-parse --is-shallow-repository`) or has no `main` ref, and judges it everywhere else. Proved three ways: passes in the full clone; still fails there with the old 3a4f6b01 restored ("not an ancestor of main"); skips in the shallow clone with "shallow checkout: the bases' reachability cannot be judged here". Module 27/27; ruff/pyright clean.
…mes out test_409_from_a_second_tab_offers_reattach_that_adopts_the_session failed on PR #24's first run (35380095596, e2e 568/569) with the same symptom c8b832f addressed on 2026-09-18: Locator.click on the Start button timed out, "element is not visible", after the settled wait it added had passed. That fix rested on init()'s state fetch landing late; this recurrence says that was not the whole mechanism. Read against the second failure: the timer has no periodic state poll, its only click-free adopt path is init()'s single fetch (which the settled wait covers — `topic` starts as 'Loading...'), the console's own load-time adoption writes nothing into the timer, and no nav.go fires without a click. Nothing found explains a hidden picker. The failure diagnostics artifact held nothing for this test either: the timeout fires before its own _diag hook. So the click now records what a third occurrence needs — the nav view, sessionActive/starting/topic/agent/resolvedTopic, the held conflict id, the picker's computed display, the button's disabled state and the URL — to a JSON beside a screenshot and DOM dump, then re-raises. No behaviour or timing changed: the fix must rest on that evidence, not on a guess. Class 7/7 locally in natural order; ruff/pyright clean.
This was referenced Sep 20, 2026
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.
Summary
Follow-on bookkeeping after items 1–4 (#20, #22) and the seam-test fix (#23) — five commits, one logical change each, RED before GREEN where a test is added:
test_plan_repair_nonactive_unready_is_noop_with_pointer(exit 0, pointer toplan architect, no launch, no write, not the ready plan's sentence) andtest_duplicate_learning_record_with_mission_revision_still_saves_once(one save, mission applied, record single). Each proved discriminating by mutating the branch it guards — the repair pin fails when the non-active branch falls through to the launch, the record pin fails whennot mission_updatesis dropped — with the sibling staying green and sources restored byte-identical.scripts/verify/plan_integration.py: the early protected-files base must exist on origin.PROTECTED_EARLY_BASE = "3a4f6b01"survived the history consolidation only as an unreachable object in one clone (git fetch origin 3a4f6b01finds no ref), soprotected-files-3a4f6b01would fail on every fresh checkout. Repointed tod7f568bf(the same commit onmainby exact subject; the three protected files byte-identical), the check renamedprotected-files-early-baseby role like the late one, and a new registry test pins that both bases are ancestors ofmain(fails on the old sha).receipts/issue-closeout-draft-2026-09-16.md's status paragraph records the seven rewritten shas and the four rows the programme had overtaken); Plan integration 1: Centralize reads and activation #8, Plan integration 2: Centralize mutations and checkpoints #9, Plan integration 4: Add MCP discovery and authoring #11, Plan integration 5: Add MCP progression and deletion #12, Plan integration 6: Launch planning-purpose agent sessions #13, Plan integration 7: Add the Web architect journey #14 closed; Plan integration 3: Make Now plan-aware end to end #10/Plan integration 8: Reconcile release contract and verify #15 open on rubric row 3b; Integrate Study Plans across Now, Web architect, and MCP #7 reopened (auto-closed by Plan integration: PlanApplication seam, Bugs A/B closed (Phases 0–1) [in progress] #20's body) with the parent mapping.tasks.mdT7.1 carries the per-step state. Stage-9 script header corrected (agents can delete remote refs; the ruleset is the gate).proposals/2026-09-16-context-derived-plan-bias.md(D-D) and…-overdue-nudge-and-retire.md(D-E), each quoting the owner's decision verbatim and stating what the engine and store do today from the code..gitignore's plan-integration allowlist gainsproposals/.learning/decision.py; three amendments recorded (demand classes are the struggle collector's own; a deferred repair needs its ownDeferredRepairin an additiveenergy_deferred_repairskey, sinceDeferredMilestoneand its three renderers are milestone-shaped; the body-double door is--mode co-study, not the read-only focus route).Tested
Both pin modules 78/78; verify-script tests 27/27 with the renamed check exiting 0 for real against
d7f568bf; six docs guard modules 211/211 with the proposals in place;mkdocs --strictexit 0;openspec validate --specs --all25/25; ruff / format / pyright clean; all pre-commit hooks on every commit. Full suite 30 failed / 5120 passed / 14 errors — the 44 failed+errored ids byte-identical to the item-4 control's committed environmental set (run − control = ∅, control − run = ∅).Merge
Local fast-forward once green (receipts cite SHAs), as with #20, #22, #23.