feat: learn subject-plugin contract implementation + French story ladder (learn uplift)#3
Merged
Merged
Conversation
Four beginner-rung stories for the French graded-reader ladder (t7a): le marché (A1), la rentrée (A1), un café entre amis (A2), un week-end à la montagne (A2). Each validates against learn-cli's story schema (schema_version 1.0): 150-220 words, 3-5 glossary entries per 100 words, 4 comprehension exercises mixing multiple_choice with an open-ended type.
Four B1-rung stories: la recherche d'appartement, le covoiturage, la fête des voisins, un entretien d'embauche. Longer (230-270 words) and grammatically richer than the A1/A2 rung (passé composé, imparfait, subjunctive, subordinate clauses); same schema and exercise-mix requirements as the rest of the ladder.
Three advanced-rung stories completing the graded-reader ladder: le télétravail (B2, essay-style), la transition écologique (B2, village profile), le vieux libraire (C1, literary short story) — 275-370 words, subjunctive/idiomatic register, still self-contained and answerable from the text alone. Adds docs/stories.md: the ladder table, the flat filename/id convention, the reproducible authoring pipeline (intended cloudai-cli batch command, marked pending since cloudai isn't installed in this environment; every story here was hand-authored to the same schema contract instead), the validation snippet against a learn-cli checkout, how to add a story, and the human-review checklist (level-appropriateness, glossary accuracy, exercise answerability, cultural naturalness, originality).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jkvh4uxSi6AsBJYPjiurze
Build the full tutor domain on the template chassis: the eight contract
verbs, XDG learner state, and committed story content — french-cli is now
the reference subject implementation that spanish-cli ports mechanically.
Verbs (all top-level via register(sub), schema-valid --json, exit 0/1/2):
- overview — reconciled: emits contract subject_overview AND keeps the
agent-first `sections` key (open payload; subject id is now `french`).
- progress — per-item mastery ladder, counters, weak items, next step.
- advice — deterministic study advice from stored state.
- story — list + read; read wraps the committed story in a directive
and sets the learner's position; unknown id -> exit 1.
- lesson — start / next / repeat (--harder bumps the difficulty rung).
- practice — item/module/lesson scope, or review (weakest touched items).
- record — the write-back; updates mastery (fail->introduced,
partial->practiced, pass->mastered, never regressing); recorded object
structurally forbids score/grade/points.
- doctor — reconciled: keeps identity checks, adds contract_version +
content/state/schema checks; exit 0 healthy, 2 unhealthy.
Engine (LLM-free, subject-agnostic except subject.py + curriculum + stories):
- french/tutor/{subject,curriculum,stories,state,engine}.py
- state persists one JSON per learner under $FRENCH_CLI_LEARN_HOME >
$XDG_DATA_HOME/french_cli/learn > ~/.local/share/french_cli/learn,
atomic writes, resumes across sessions.
Content: 3 dev stories at content/stories/dev-*.json (glob-discovered, no
hardcoded names/counts) + a 3-module / 5-lesson / 9-item curriculum.
Cite-don't-import: french/contract_cite/ (11 schemas + stdlib validator,
verbatim from learn-cli ed81b87, contract v1.0); docs/contract-provenance.md.
Tests: +57 (23 -> 80): contract conformance per verb, state round-trip +
mastery inference, story content validation (globs content/stories),
behavioural CLI. Coverage 88.6%.
Gates: pytest, black, isort, flake8, bandit, markdownlint, and
`teken cli doctor . --strict` all green. Wheel force-includes content so
the published CLI resolves stories without the repo checked out.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Jkvh4uxSi6AsBJYPjiurze
…, state, curriculum) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jkvh4uxSi6AsBJYPjiurze
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jkvh4uxSi6AsBJYPjiurze
|
PR Summary by QodoImplement learn subject-plugin contract + French graded-reader story ladder
AI Description
Diagram
High-Level Assessment
Files changed (57)
|
Code Review by Qodo
Context used✅ Tickets:
🎫 Implement the learn subject-plugin contract + story ladder (unblocks agentculture.org/learn)✅ Compliance rules (platform):
7 rules✅ Skills:
doc-test-alignment 1. Changelog entries lack rationale
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+12
to
+15
| - Learn subject-plugin contract v1.0: eight tutor verbs (overview, progress, advice, story, lesson, practice, record, doctor) with schema-valid --json and contractual exit codes | ||
| - Tutor engine: 3-module French curriculum (5 lessons, 9 items, 18 exercises), per-learner XDG state with atomic writes and resume | ||
| - Story ladder: 11 graded-reader stories (A1-C1) with glossaries and comprehension exercises, plus dev fixtures; CI validates all content against the cited story schema | ||
| - Cited contract schemas + validator from learn-cli (contract v1.0, provenance ledger in docs/contract-provenance.md) |
There was a problem hiding this comment.
1. Changelog entries lack rationale 📘 Rule violation ⚙ Maintainability
The newly added 0.5.0 CHANGELOG entries are single-sentence/fragments and do not include a second sentence explaining why the change was made. This violates the requirement that each new changelog entry include a multi-sentence rationale.
Agent Prompt
## Issue description
New `CHANGELOG.md` entries must contain at least two sentences: one describing what changed and a second explaining why.
## Issue Context
PR Compliance ID 1864313 requires multi-sentence rationale entries for each new or modified changelog bullet.
## Fix Focus Areas
- CHANGELOG.md[8-16]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+108
to
+113
| root = content_dir() | ||
| if root is None: | ||
| return None | ||
| path = root / f"{story_id}.json" | ||
| if not path.is_file(): | ||
| return None |
There was a problem hiding this comment.
2. Story path traversal read 🐞 Bug ⛨ Security
french.tutor.stories.read_story() builds a filesystem path from unvalidated story_id, so inputs containing .. or path separators can escape the stories directory and read arbitrary readable .json files. french story read <id> passes user input directly to this function, bypassing the contract’s restricted story-id pattern.
Agent Prompt
## Issue description
`read_story(story_id)` joins user-controlled `story_id` into a `Path` (`root / f"{story_id}.json"`) without validating the ID or ensuring the resulting path stays inside the intended content directory. This enables `../` traversal to read other `.json` files.
## Issue Context
The contract’s story ids are restricted to `^[a-z0-9][a-z0-9._-]*$` (no slashes), but the CLI does not enforce this before filesystem access.
## Fix Focus Areas
- Validate `story_id` against the contract regex and reject early (return `None` or raise a `CliError` upstream).
- Ensure the resolved path is inside `content_dir()` (e.g., `resolved.is_relative_to(root.resolve())` or equivalent guard).
- Consider adding the same validation in the CLI argument layer for faster feedback.
### Fix Focus Areas (code)
- french/tutor/stories.py[84-117]
- french/cli/_commands/story.py[39-46]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+303
to
+331
| if result not in RESULTS: | ||
| raise ValueError(f"result must be one of {', '.join(RESULTS)}") | ||
| if activity not in ACTIVITIES: | ||
| raise ValueError(f"activity must be one of {', '.join(ACTIVITIES)}") | ||
| if mastery is not None and mastery not in MASTERY_LEVELS: | ||
| raise ValueError(f"mastery must be one of {', '.join(MASTERY_LEVELS)}") | ||
| if total is not None and total < 1: | ||
| raise ValueError("total must be >= 1 when given") | ||
| if correct is not None and correct < 0: | ||
| raise ValueError("correct must be >= 0 when given") | ||
|
|
||
| recorded: dict[str, Any] = { | ||
| "item_id": item_id, | ||
| "activity": activity, | ||
| "result": result, | ||
| "at": now_iso(now), | ||
| } | ||
| if exercise_id: | ||
| recorded["exercise_id"] = exercise_id | ||
| if story_id: | ||
| recorded["story_id"] = story_id | ||
| if lesson_id: | ||
| recorded["lesson_id"] = lesson_id | ||
| if correct is not None: | ||
| recorded["correct"] = correct | ||
| if total is not None: | ||
| recorded["total"] = total | ||
| if duration_seconds is not None: | ||
| recorded["duration_seconds"] = duration_seconds |
There was a problem hiding this comment.
3. Negative duration breaks schema 🐞 Bug ≡ Correctness
state.record_result() stores duration_seconds without enforcing the contract schema’s `minimum: 0, so record_ack` can become schema-invalid while still exiting success. This violates the “schema-valid --json” contract and can break downstream consumers/conformance.
Agent Prompt
## Issue description
The contract schema requires `recorded.duration_seconds >= 0`, but `state.record_result()` accepts and persists negative values. This can produce schema-invalid `record_ack` output while still returning exit code 0.
## Issue Context
The CLI accepts `--duration-seconds` as a float with no bounds checks and passes it through to `state.record_result()`.
## Fix Focus Areas
- Add validation in `state.record_result()`:
- if `duration_seconds is not None and duration_seconds < 0: raise ValueError(...)`
- (Optional) add an argparse type/validator for `--duration-seconds` so user error is caught at parse time.
### Fix Focus Areas (code)
- french/tutor/state.py[281-333]
- french/cli/_commands/record.py[64-101]
- french/contract_cite/schemas/record.json[31-46]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+84
to
+106
| def list_summaries(level: str | None = None) -> list[dict[str, Any]]: | ||
| """Story summaries for the catalog, optionally filtered by coarse ``level``. | ||
|
|
||
| Malformed files are skipped rather than crashing the whole listing; the | ||
| content-validation CI test is what fails the build on a bad file. | ||
| """ | ||
| out: list[dict[str, Any]] = [] | ||
| for path in story_files(): | ||
| try: | ||
| story = load_story(path) | ||
| except (json.JSONDecodeError, ValueError, OSError): | ||
| continue | ||
| if level is not None and story.get("level") != level: | ||
| continue | ||
| out.append(_summary(story)) | ||
| return out | ||
|
|
||
|
|
||
| def read_story(story_id: str) -> dict[str, Any] | None: | ||
| """Return the full committed story object for ``story_id``, or ``None``. | ||
|
|
||
| Lookup is by filename stem (the pinned ``filename == id`` rule), so it is a | ||
| direct file open, not a directory scan. |
There was a problem hiding this comment.
4. Story list emits invalid 🐞 Bug ≡ Correctness
stories.list_summaries() does not schema-validate parseable story JSON and _summary() fills missing required fields with empty strings, which can produce story_list payloads that violate story_list.json while still exiting 0. This breaks the contract guarantee of schema-valid --json when story content is schema-invalid-but-parseable (including via $FRENCH_CLI_CONTENT_DIR).
Agent Prompt
## Issue description
`list_summaries()` only skips JSON parse/IO errors; it does not run `validate_story()`. `_summary()` then defaults missing required fields (`id`, `title`, `level`) to `""`, which can violate the `story_list` schema.
## Issue Context
The repo has CI validation for committed stories, but runtime also supports `$FRENCH_CLI_CONTENT_DIR` overrides and can encounter corrupted/modified content; the CLI should still avoid emitting contract-invalid JSON.
## Fix Focus Areas
- In `list_summaries()`, run `validate_story(story)` and skip entries with validation errors.
- Avoid defaulting required fields to empty strings; either skip invalid stories or raise a clean `CliError` in strict modes.
- Consider validating `read_story()` outputs too (return `None` / error on schema-invalid story object).
### Fix Focus Areas (code)
- french/tutor/stories.py[71-117]
- french/contract_cite/schemas/story_list.json[14-30]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+157
to
+194
| def _coerce(raw: dict[str, Any], learner: str) -> dict[str, Any]: | ||
| """Typed, identity-safe projection of ``raw`` onto the current schema.""" | ||
| embedded = raw.get("learner") | ||
| if embedded is not None and embedded != learner: | ||
| raise StateError(f"state file for '{learner}' carries a different learner ({embedded!r})") | ||
| base = new_state(learner) | ||
| for key, expected in _FIELD_TYPES.items(): | ||
| if key in raw: | ||
| if not isinstance(raw[key], expected): | ||
| raise StateError(f"state field '{key}' has the wrong type") | ||
| base[key] = raw[key] | ||
| base["learner"] = learner | ||
| base["subject"] = subject.SUBJECT_ID | ||
| base["state_version"] = STATE_VERSION | ||
| return base | ||
|
|
||
|
|
||
| def load(learner: str) -> dict[str, Any]: | ||
| """Load a learner's state, or a fresh default if none exists. | ||
|
|
||
| Raises :class:`StateError` on a corrupt file or a *newer* state version. | ||
| """ | ||
| path = state_path(learner) | ||
| if not path.is_file(): | ||
| return new_state(learner) | ||
| try: | ||
| raw = json.loads(path.read_text(encoding="utf-8")) | ||
| except (json.JSONDecodeError, OSError) as exc: | ||
| raise StateError(f"cannot read state file {path}: {exc}") from exc | ||
| if not isinstance(raw, dict): | ||
| raise StateError(f"state file {path} is not a JSON object") | ||
| version = raw.get("state_version", STATE_VERSION) | ||
| if isinstance(version, int) and version > STATE_VERSION: | ||
| raise StateError( | ||
| f"state file {path} has state_version {version}, " | ||
| f"newer than this CLI supports ({STATE_VERSION})" | ||
| ) | ||
| return _coerce(raw, learner) |
There was a problem hiding this comment.
5. State file not refused 🐞 Bug ☼ Reliability
state.load() only rejects future state_version when it’s an int, and _coerce() overwrites state_version/subject without validating them or nested value types. Malformed state can pass load() and later crash commands (e.g., lesson repeat calling int() on repeats values) instead of raising StateError and returning a clean environment error.
Agent Prompt
## Issue description
The state loader intends “tolerant-but-typed loads”, but it does not enforce:
- `state_version` type (non-int values bypass the future-version refusal)
- `subject` consistency
- nested invariants/types for `repeats` (and other nested structures)
This allows malformed state to reach code paths that assume numeric values and crash.
## Issue Context
`_tutor.load_state()` converts `StateError` into a clean `EXIT_ENV_ERROR`, but permissive loading means truly-bad state can still crash later (e.g., `repeat_difficulty()` calling `int()` on a non-numeric string).
## Fix Focus Areas
- Enforce `state_version` must be an `int`; if present and not int, raise `StateError`.
- Validate embedded `subject` (if present) matches `subject.SUBJECT_ID`.
- Validate `repeats` values are ints (or coercible) during load/coerce; otherwise raise `StateError`.
- Consider validating `mastery` values are within `MASTERY_LEVELS` on load to prevent later schema-invalid `progress` output.
### Fix Focus Areas (code)
- french/tutor/state.py[145-195]
- french/tutor/state.py[266-275]
- french/cli/_commands/lesson.py[70-82]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
1 participant
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.
What
french-cli becomes the reference subject-plugin implementation for the learn platform (closes #2):
overview,progress,advice,story list/read,lesson start/next/repeat,practice,record,doctor— all schema-valid--json, exit codes 0/1/2, directive pattern (the CLI is LLM-free; the driving agent tutors and records back).docs/contract-provenance.md).Conformance:
learn subject doctor french→ healthy, 8/8 checks (verified against learn-cli's gate).Tests 23 → 113; coverage 88.6%; all gates green (pytest, lint, markdownlint,
teken cli doctor --strict).Built by the learn-uplift workforce (tasks t4 + t7a), TDD-gated merges. Spec: agentculture/learn-cli
docs/specs/.Note for maintainer: local branch
docs/init-claude-mdpredates this work and remains unmerged (see #2).🤖 Generated with Claude Code
https://claude.ai/code/session_01Jkvh4uxSi6AsBJYPjiurze