Skip to content

Plan integration: PlanApplication seam, Bugs A/B closed (Phases 0–1) [in progress] - #20

Draft
NetDevAutomate wants to merge 21 commits into
mainfrom
fix/plan-integration-bugs
Draft

NetDevAutomate wants to merge 21 commits into
mainfrom
fix/plan-integration-bugs

Conversation

@NetDevAutomate

Copy link
Copy Markdown
Owner

Draft so CI runs on every push, following the #18/#19 convention. Not ready to merge until Phase 6 (#15).

What this branch is

Closes the two bugs issue #7 named as must-fix-first and lands the PlanApplication seam (#8) they exist because of. Tracked in openspec/changes/plan-application-seam/ (proposal, design, tasks with a per-task definition of done) and the council record under docs/architecture/plan-integration/council/.

  • Bug Bevaluate_and_record discarded record_checkpoint's boolean; a failed DB write reported warnings=[]. Fixed at the caller (D-1).
  • Bug A — activation readiness was gated on one Web door (PATCH status) and bypassed on create-with-status and whole-document replacement. Closed by the seam: one gate on the resulting document, one save (D-2). Council review 1 then found a fourth door (compound PATCH = transition + second unguarded save) and a fifth (field-only edit of an active plan); both closed by bringing RevisePlan forward.
  • New modules planning/{errors,views,intents,application}.py; Web and CLI list/inspect/activate/create/replace/revise routed through it; the route file has zero readiness()/save_plan calls.

Evidence

  • RED before code on every task; the three protected suites (test_web_plans.py, test_cli_plan.py, test_planning_evaluation.py) are byte-identical to the RED commit.
  • Full suite 4629 passed, 4 skipped; just lint, just typecheck clean.
  • Council code review 1: GPT Astra REJECT (fourth door, reproduced by hand before acceptance) → fixed; Grok 4.6 ACCEPT-WITH-CORRECTIONS; qwen3-coder. Arbitration: review-1-arbitration-2026-09-15.md.

Still to land on this branch

Phase 2 (#9: SetMilestone, DeletePlan, assess, get_active_guidance, AST architecture guard, remaining CLI migration) → Phase 3 (#10 plan-aware now#11 six MCP tools ∥ #13a planning purpose) → Phase 4 (#12 ∥ #13b) → Phase 5 (#14 Web architect) → Phase 6 (#15 reconcile; closes #7#15).

Related: #7 #8 #9 #10 #11 #12 #13 #14 #15

…x-first

Three failing tests, written from the spec invariants rather than from the
code, so the fix has a definition of done before it exists:

- POST /api/plans with status=active on an unready plan currently returns 201
  and persists an active plan whose own readiness says ready=false with three
  blockers. Spec #7 invariant: activation is readiness-gated on EVERY entry
  path; only the PATCH status path was gated.
- PATCH /api/plans/{id} with a whole-document markdown replacement whose
  frontmatter says active and has no milestones currently returns 200.
  Same invariant, third door.
- evaluate_and_record() discards record_checkpoint()'s boolean. The function
  swallows its own failures and returns False, so a failed DB write produced
  an evaluation with warnings=[] — complete recording claimed after a
  partial write (issue #9 acceptance criterion).

Two companion tests pass today and pin the non-regression side (a ready
plan still activates; a successful DB write adds no warning).
…, arbitration, openspec change

Why: issues #7#15 (Study Plan integration) were specified on 2026-09-04 and
nothing landed; two of the bugs the parent issue names as must-fix-first are
now RED-tested at 3a4f6b01. The owner asked for the outstanding work to be
planned by a council of models (GPT Astra, Grok 4.6, one best-for-purpose
seat) and executed TDD with a per-task definition of done.

What lands:
- scripts/council/run_council.py — fans one brief out to N gateway models in
  parallel, one receipt per seat + manifest (brief sha256, latency, tokens).
  scripts/council/system-seat.md — the no-tools seat contract, added after
  Grok's first run announced "I'll inspect the repo" and looped for 20k
  tokens (the invalid receipt is kept, renamed, as the instrument record).
- council/brief-plan-2026-09-15.md and the three seat receipts.
- council/arbitration-plan-round1-2026-09-15.md — decisions D-1..D-17 with the
  rejected alternatives named per seat, plus the facts the seats flagged as
  unknown, now verified in the tree (CLI status is already gated; energy_floor
  exists; previous_notes renders a "Resuming" section so it is the wrong
  carrier for a planning brief).
- openspec/changes/plan-application-seam/{proposal,design,tasks}.md — the
  work order: phases, owners, files, RED test names, command-checkable DoD,
  and the council review gates.
- .gitignore: un-ignore docs/architecture/plan-integration/ (same shape as
  the session-memory block; delivered HTML stays ignored).
- .pre-commit-config.yaml: detect-secrets excludes council manifest.json —
  they hold sha256 digests of committed public text, the same false positive
  the UAT registry exclusion already documents.
`index.record_checkpoint` reports failure two ways: it swallows its own
errors and returns False (no database, INSERT failed), and it can still
raise from an import or connection fault. `evaluate_and_record` only
handled the raise, so a False return produced an evaluation with
warnings=[] — complete recording claimed after a partial write. The RED
test committed at 3a4f6b01 (`test_failed_checkpoint_db_write_is_reported_
as_a_warning`) pinned exactly this.

Both paths now append the existing "checkpoint not saved to the database"
string, so callers reading an empty warning list can trust the checkpoint
is durably recorded. The index's swallow-and-return-False stays as its
best-effort policy (council D-1); the fix lives in the one caller that
was ignoring the answer.

Decision: D-1 (arbitration-plan-round1-2026-09-15).
Twenty-two failing tests for the seam that closes Bug A, written against
design §1 and decisions D-2/D-3/D-4 rather than against any adapter. On
this tree the module does not exist, so the file fails at import
(ModuleNotFoundError: studyloop.planning.application).

The load-bearing invariant is that activation is readiness-gated on EVERY
entry path. Four doors are pinned — create-with-status, lifecycle
transition, whole-document replacement, and document import — and one
test asserts that all four refuse with the identical ReadinessView payload
and write nothing first. The fourth door (ImportDocument) is not in the
design's intent list: POST /api/plans has a raw-markdown import branch
that would otherwise stay an ungated path into "active" or have to keep a
route-local readiness gate, which D-2 forbids. It is a create, so it ships
in Phase 1 with the other create door.

Also pinned: browse ordering equals store.list_plans (active first, then
ascending updated, then id) so the Web list and CLI table do not reorder
when they migrate; PlanSummary/ReadinessView serialise byte-for-byte to
StudyPlan.summary() and authoring.readiness() (D-3), which is what keeps
the REST bodies unchanged; views are frozen, tuple-only, and to_json_dict()
hands every caller a fresh container.

The file carries a RED-commit-only pyright directive: the workspace
pre-commit hook type-checks tests, and unresolved imports would otherwise
make a test-before-code commit impossible without skipping hooks. T1.2
removes the directive when the modules exist.

Ticks T0.1 in the tasks file with its sha (c16ffa35).
…oor (T1.2)

Four new modules under studyloop/planning, per design §1 and D-3:

  errors.py       PlanError + PlanNotFound, InvalidPlanId, PlanConflict,
                  InvalidField, PlanNotReady(readiness), InvalidMilestone.
                  Exceptions with no CLI/HTTP/MCP vocabulary; adapters map
                  them once. Names are the arbitration's (no `Error`
                  suffix — the suffixed forms already exist in store.py with
                  stdlib bases and are what the store raises *to* the seam).
  views.py        Frozen, tuple-only read models; to_json_dict() builds a
                  fresh container per call. PlanSummary and ReadinessView
                  serialise to StudyPlan.summary() and authoring.readiness()
                  key for key, so REST bodies do not change.
  intents.py      CreatePlan, ImportDocument, ReplaceDocument,
                  TransitionLifecycle — the four ways a document can become
                  active. `overwrite` stays on CreatePlan/ImportDocument for
                  the Web/CLI request shapes (D-4); MCP will not expose it.
  application.py  browse / inspect / prepare_planning / apply. `apply` runs
                  the readiness check whenever the RESULTING document would
                  be active and raises PlanNotReady before any write.

Why a seam rather than a shared helper: the two Bug A doors exist because
the gate lived on one Web route. A helper called from each route is the
same duplication with a function name (D-2 rejected exactly that). Here
the adapters never hold a StudyPlan to mutate, so a door cannot forget
the gate — it has no way to write except through `apply`.

Deviations from design.md, each deliberate:
- ImportDocument is an eighth intent. POST /api/plans has a raw-markdown
  import branch that is also a create-and-activate door; without an intent
  it would stay ungated or keep a route-local gate.
- ReadinessView carries plan_id; MilestoneView carries notes; PlanDetail
  carries learning_records, resources and the document's checkpoints, with
  `history` (the DB log) behind include_history. All so the existing GET
  body serialises unchanged (D-3 outranks the sketch).
- No `plans_dir` constructor argument: directory resolution stays with
  store.plans_dir() (env var / settings), as every fixture already relies
  on. Threading a base path through eleven store functions for a parameter
  no adapter passes would widen the diff for no caller.

Removes the RED-only pyright directive from test_plan_application.py.
23 tests green; pyright 0 errors on studyloop/planning.
…ate deleted (T1.3)

Closes Bug A. POST /api/plans (both the interview and the raw-markdown
branch), PATCH markdown, PATCH status, GET list/detail/markdown/history
and GET interview now go through the seam. The readiness check that lived
only on the PATCH-status branch is gone from this file — `rg 'readiness\('`
finds nothing — because every door into "active" is gated once, inside
`apply` (D-2: delete the route gate, do not add a third copy).

The two RED tests from 3a4f6b01 go green: an unready create-with-status
and an unready active document replacement both return 422 with the same
body the PATCH-status refusal always had ({"message", "plan_id", "ready",
"blockers", "nudges"}) and persist nothing. Every pre-existing assertion
in test_web_plans.py is unchanged (git diff against 3a4f6b01 is empty)
and passes: 27/27.

Domain errors map to status codes in one function (design §2):
PlanNotFound 404, InvalidPlanId/InvalidField 400, PlanConflict 409,
PlanNotReady 422, InvalidMilestone 404.

PATCH ordering: existence (404) → validate every field edit (400) →
lifecycle transition (400/422) → field edits → save. Previously the
readiness 422 was checked before the title/energy/milestone 400s; now a
body that is both unready-active and carries a bad field gets the 400.
Both refuse without writing. The alternative — transition first — would
persist the status change before a 400 on a sibling field, which the
single save_plan never did.

Still on direct imports until Phase 2 (AssessPlan, RevisePlan,
SetMilestone, DeletePlan): GET/POST evaluate, PATCH field/milestone
edits, the milestone toggle and DELETE.
…cation (T1.4)

`plan list` browses, `plan show` inspects (with the raw document only when
--markdown asks for it), and `plan status` applies a TransitionLifecycle
intent. The CLI's own readiness check before `status … active` is gone:
the refusal now comes from the seam's single gate, so the message a
learner sees in the terminal — "Cannot activate 'x' — the plan is
incomplete." followed by the blockers and nudges — is produced from the
same ReadinessView the Web API turns into its 422 body. That is what the
parity test in T1.5 asserts.

`_print_readiness` consumes a ReadinessView; `_refuse_activation` is the one
place the "Cannot activate" copy lives (it was duplicated between `new
--activate` and `status`). `plan new` still drafts and creates directly —
it moves onto CreatePlan in Phase 2 — but builds its ReadinessView from the
draft so the output path is already the shared one.

Exit codes and output are unchanged: test_cli_plan.py is byte-identical to
3a4f6b01 and passes 22/22. pyright 0 errors on cli/_plan.py.
…identically (T1.5)

Two tests that meet the invariant from the outside, the way a learner or an
agent does, rather than through the seam's own API:

- test_activation_refusal_is_identical_via_cli_and_web: one unready draft;
  `studyloop plan status … active` exits 1 and its bullet list is exactly
  the Web PATCH 422 body's blockers followed by its nudges, in order; the
  document on disk is byte-identical afterwards, `plan show --json` still
  says draft and reports the same readiness the Web refused with, and the
  active listing is empty. This one already held on 3a4f6b01 — both
  surfaces gated the transition door — so it pins parity rather than
  reproducing a bug.

- test_every_web_door_into_active_refuses_with_the_same_body: create-with-
  status, status transition, whole-document replacement and raw-markdown
  import all return 422 with equal bodies (plan_id aside for the import,
  which names its own). RED on the 3a4f6b01 adapters (create returned 201);
  green on the seam.

RED evidence was taken by checking out the pre-seam web/routes/plans.py and
cli/_plan.py into the working tree against the current planning package
(1 failed, 1 passed), then restoring HEAD (2 passed).
…s + public doc (T1.6)

Delta specs for the plan-application-seam change, in the repo's
ADDED/Requirement/Scenario shape, one per capability the seam touches:

- web-ui: the routes hold no readiness check; every door into "active"
  (create-with-status, document replacement, status transition, raw
  import) returns the same 422 body and writes nothing; the seam→HTTP
  error mapping is stated once; REST bodies are unchanged.
- cli-surface: `plan status … active` applies TransitionLifecycle and its
  bullet list is the Web body's blockers then nudges; list/show read
  through the seam with their --json shapes unchanged.
- active-learning-decisions: the seam itself — the single gate before any
  canonical write, frozen views that serialise to the existing key sets,
  domain exceptions with no adapter vocabulary — plus the Bug B rule that
  a False from record_checkpoint is reported exactly like a raise.

Each scenario corresponds to a test that exists on this branch
(test_plan_application.py, test_web_plans.py, test_cli_plan.py,
test_plan_surface_parity.py, test_planning_evaluation.py).
`openspec validate plan-application-seam` passes.

docs/study-plans.md gains an "Activation" section saying what the gate
requires, that it runs on every route into active on every surface, and
that a refusal writes nothing. The "What a plan does not do yet" list is
untouched: nothing in Phase 1 changes what a plan does, only how safely it
becomes active.

Ticks T1.1–T1.5 in the tasks file with their shas.
Recorded from command output on 2ca6bdb0: `just lint` clean (ruff check +
format --check over 1011 files), `just typecheck` 0 errors workspace-wide,
`pytest packages/studyloop/tests -k "plan or planning"` 346 passed exit 0,
and the full studyloop suite 4576 passed / 4 skipped / 0 failed exit 0.

Phase 1 stops here per the work order; Council review 1 gates Phase 2.
…urrent release)

The release-consistency check wants every openspec change with commits since
the last tag either archived or carrying an explicit deferred reason. Phases
0-1 have landed; Phases 2-6 remain. Archive when #15 closes.
…g-document gate)

Council code review 1 (GPT Astra REJECT, qwen3-coder's own red finding
agrees) found that the Phase 1 route composes a seam transition with a
second, unguarded save, so two doors into active-but-unready survived:

- F1  PATCH {"status":"active","milestones":[]} on a ready draft -> 200,
      stored active with 0 milestones, ready=false. Its mirror is also
      wrong today: adding the missing milestones in the same request is
      refused (422) because readiness is judged on the pre-edit document.
- F1b PATCH {"milestones":[]} on an already-active plan -> 200, leaving an
      active plan that cannot be evaluated. This one predates the branch.
- F4  POST with a duplicate id AND an unready active body -> 422; the
      delta spec's "Duplicate id without overwrite" scenario says 409
      unconditionally (identity before readiness).

Reproduced by hand against ac121874 before writing these; all four fail
on this tree. The fix is RevisePlan brought forward from Phase 2: one
load, all edits applied to a candidate, readiness judged on the result,
one save — never a route-side mutation after apply().
…n the resulting document

Council review 1 (seat openai.gpt-6-astra, finding F1, 🔴) rejected Phase 1
because `PATCH /api/plans/{id}` composed a seam transition with a second,
unguarded route-side save: `{"status": "active", "milestones": []}` was
gated against the OLD milestones, saved as active, and then had its
milestones stripped — an active-but-unready plan, the very state issue #7
exists to prevent. The mirror case (an unready draft supplied with its
missing milestones in the same request) was refused before those milestones
were considered, and (F1b) a field-only `{"milestones": []}` against an
already-active plan bypassed the seam entirely. Two saves also meant a
failed second write left the status change committed.

Bring `RevisePlan` forward from Phase 2 with design §1's explicit fields
(title, topics, target_date, energy_floor, review_cadence_days, notes,
milestones, learning_record) plus `status`, so a compound PATCH is ONE
intent. `_revise` loads once, validates every field before applying any
(404 before 400; a bad field beside a good status change writes nothing),
applies them to the candidate, runs `_assert_can_be_active` whenever the
RESULTING document is active — whether `status` makes it so or the plan
already is — and saves once. `plan_id` and `created` are preserved and
`updated` is bumped by the store's single save. `TransitionLifecycle` is
now the one-field case of a revision, so there is exactly one gate path.

The route only translates the body into the intent: `_field_updates` and
the route-side `save_plan` are gone. Field validation moved into the seam
as `InvalidField` with the same messages the route used ("title cannot be
empty", "milestones must be a list", "energy_floor must be an integer"), so
the existing 400 assertions in test_web_plans.py are unchanged. The
milestone toggle is expressed as a full-list `RevisePlan` until Phase 2's
`SetMilestone`, which removes the last route-side write:
`rg 'readiness\(|save_plan' web/routes/plans.py` → 0 hits.

`learning_record` mirrors `store.record_learning`'s rules (empty title and
H1-H3 body lines refused; identical title+body is an idempotent no-op) on
the in-memory candidate so the revision stays one save; the store writer
remains for the CLI/MCP paths Phase 2 migrates.

RED→GREEN: the four parity tests committed at 8d11ee40 for F1/F1-mirror/F1b
now pass; new seam tests in test_plan_application.py pin one save per
compound revision (monkeypatched `store.save_plan` call count == 1),
id/created preservation, the active-but-would-be-unready refusal, and that
invalid fields raise before any write.
…ment is judged

Council review 1, finding F4 (🟡): `_persist_new` ran the readiness gate
before the store's duplicate-id check, so `POST /api/plans` with an id that
already exists AND `status: active` on an unready body answered 422 rather
than the 409 the delta spec's "Duplicate id without overwrite" scenario
promises unconditionally. Both outcomes wrote nothing, but the API's
precedence was asserted in one place and contradicted in another.

Order identity, conflict, readiness, write: validate the id (a traversal id
is an `InvalidPlanId`, never a readiness refusal), probe the plans directory
for the id and raise `PlanConflict` unless `overwrite` was set, then gate,
then create. The store's own `PlanExistsError` is still translated so the
race between the probe and the write stays a conflict.

RED→GREEN: parity test
`test_duplicate_id_is_a_conflict_even_when_the_new_document_is_unready_active`
(committed RED at 8d11ee40) passes; seam tests
`test_duplicate_unready_active_create_reports_conflict` (parametrised over
create-with-status, import by frontmatter id, import by explicit id) and
`test_malformed_explicit_id_is_refused_before_readiness` pin the precedence
for every create door.
…maps every refusal

Council review 1, finding F3 (🟡, seat openai.gpt-6-astra) and the qwen3-coder
seat's "CLI error mapping incompleteness": two gaps in the promise that a
domain error is translated exactly once per adapter.

Seam: `inspect` read the raw document with `store.load_plan_text` *after*
`_load` had translated the parse, so a plan deleted between the two calls
escaped as the store's `LookupError` past every adapter's `except PlanError`.
`_load_text` now applies the same translation as `_load`.

CLI: `plan list` called `browse` with no `PlanError` handler, and `_fail_for`
knew only `PlanNotFound` — `PlanConflict`, `InvalidField`, `InvalidPlanId`
and `InvalidMilestone` fell through to the bare exception text, and
`PlanNotReady` was handled at one call site rather than in the mapping.
`_fail_for` now gives each its own one-line message (design §2) and owns the
`PlanNotReady` → `_refuse_activation` case, so `plan status` needs one
`except`; `plan list` routes its refusal through it. Exit codes and the
existing "Cannot activate" / "No study plan with id" lines are unchanged —
test_cli_plan.py is untouched and green.

RED→GREEN: `test_inspect_markdown_translates_store_not_found_after_initial_load`
(seam), `test_plan_list_domain_refusal_exits_without_traceback` and
`test_cli_maps_each_seam_refusal_to_a_specific_message` (parametrised over
the five refusals) in test_plan_surface_parity.py.
… by one factory

Council review 1, finding F2 (🟡): the module docstring claimed views are
"deep-frozen on construction", but only `PlanningBrief.build()` froze the
evidence seed. The generated constructor accepted a mutable mapping as-is,
so `seed["notes"].append(...)` after construction changed a frozen view, and
`_freeze` returned unsupported leaves (a model, a bytearray, an arbitrary
object) unchanged — mutable objects a "frozen" view could not vouch for.

Freeze and defensively copy `evidence_seed` in `__post_init__` via
`object.__setattr__` (the sanctioned way for a frozen dataclass to normalise
its own fields), and normalise `interview`/`existing_plans` to tuples there
too. `_freeze` recurses through nested mappings and sequences, copying as it
goes, and raises `TypeError` for any leaf that is not a JSON scalar (str, int,
float, bool, None) — the only leaf types the seed readers produce and the
only ones `json.dumps` on the CLI path ever accepted. `build()` stays as a
convenience factory over the plain dicts the authoring module returns.

RED→GREEN in test_plan_application.py:
`test_planning_brief_direct_constructor_defensively_freezes_seed`,
`test_planning_brief_nested_seed_mutation_cannot_change_view`,
`test_planning_brief_json_calls_do_not_share_nested_containers`,
`test_planning_brief_rejects_unsupported_mutable_seed_leaf` (object,
bytearray, model; and a non-mapping seed).
… the file on every write

Council review 1, finding F5 (🟡): `ImportDocument` let an explicit id
override the frontmatter (accepted deviation 1) but never implemented its
documented final fallback. `parse_plan` resolves a missing frontmatter id to
the bare title slug, so a second import of an untitled-by-id document was a
`PlanConflict` where the pre-seam route (and `CreatePlan`) allocated
`unique_plan_id`'s `-2`, `-3`… The successful import paths — explicit-id
override actually persisting under the override, `created` preservation, a
ready `active` import, a ready `active` replacement — had no coverage.

`_import` now settles identity before the readiness gate, in order: explicit
`plan_id`, else the frontmatter id, else a unique title slug. The parser is
handed a sentinel fallback that cannot pass `validate_plan_id`, so "no
frontmatter id" is distinguishable from a real one and can never be filed.

`_load` pins the returned model to the *storage* id. The parser lets a
document's own frontmatter `id` win over the filename, so a hand-edited plan
under `target.md` whose frontmatter said `id: other` was re-saved by replace,
revise and transition as `other.md` — a second file, with `target.md` left
untouched. Every write path loads through `_load`, so "the id is the file"
now holds for all of them; `_replace` keeps preserving `created`.

RED→GREEN in test_plan_application.py:
`test_import_without_id_allocates_unique_title_slug` and
`test_replace_keeps_requested_storage_identity_when_frontmatter_disagrees`
(parametrised over replace / revise / transition; done-criterion: one updated
target document, no second file). Pinned as passing:
`test_import_explicit_id_overrides_frontmatter_without_creating_old_id`,
`test_import_preserves_document_created`, `test_ready_active_import_succeeds`,
`test_ready_active_replacement_succeeds`.
…int database

Council review 1, finding F6 (🟡): the Bug B fix (T0.1, c16ffa35) shipped
without the regression tests that pin it, and the seam tests isolated the
plans directory but not visibly the checkpoint database —
`test_inspect_carries_markdown_and_history_only_on_request` asserted an
empty history for "demo" against whatever the suite's shared database held.

New `tests/test_plan_recording_failures.py` runs every test against its own
`STUDYLOOP_DB` and plans directory and asserts both the returned evaluation
and each sink's outcome independently: a `record_checkpoint` that returns
`False` or raises adds exactly the database warning and still appends the
checkpoint to the document; a successful record adds no database warning
and leaves exactly one row; a failed document write (a raising `save_plan`)
adds only the document warning and does not discard the row the database
already holds; `append_to_plan=False` skips the document sink silently; and
the evaluation is returned even when both sinks fail (D-1/D-3: no
`PartialRecording`). Mutation check: reverting the boolean handling the way
the original bug did fails three of the six.

test_plan_application.py gains the same per-test database fixture and
`test_inspect_history_is_newest_first_and_honours_the_limit`, which seeds
three rows through `index.record_checkpoint` and reads them back through
`inspect(include_history=True, history_limit=…)`: newest first, the limit
honoured, the six-key row shape serialised, another plan's log empty.

The three protected legacy test files are untouched.
…thing (Web)

The seam test test_revise_invalid_field_raises_before_any_write already
covers RevisePlan(status="active", title="") → InvalidField with no
save; this pins the same fact end to end through PATCH so the delta spec's
new scenario "A bad field beside a status change writes nothing" has a
test a reviewer can run: 400 with the legacy message, status still draft,
document bytes unchanged.
…Activation paragraph, F1–F6 ticked

GPT Astra's §3 spec/doc review found the delta spec did not exactly match
the code and the public paragraph over-promised.

web-ui delta: the requirement now names in-place revision (including a body
that combines a status change with field edits) among the doors it gates,
says the gate judges the document "as it would be saved" whether the request
makes the plan active or it already is, requires no route-side store write
(`rg 'readiness\(|save_plan'` → 0), and expresses the refusal as the full
response `{"detail": {...}}` with `plan_id` kept — the old route included it,
so design §2's shorter sketch is corrected rather than used to drop a legacy
key. New scenarios: compound PATCH refused as one resulting document;
compound PATCH that supplies what was missing activates in one write;
field-only edit cannot make an active plan unready; ready raw-Markdown import
succeeds; conflict is judged before readiness on every create door; a bad
field beside a status change writes nothing. Each has a test in
tests/test_plan_surface_parity.py or tests/test_plan_application.py.

cli-surface delta: a requirement for the complete `_fail_for` mapping (six
domain errors, one line each, exit 1, no traceback; `plan list` routed
through it), with the two scenarios the parity tests pin.

docs/study-plans.md: the "Activation" paragraph is replaced by the bounded
wording from the review — application-mediated Web writes, CLI activation
commands, refused activation writes nothing, several plans may be active.
The old text claimed "a plan never appears active while it cannot be
tracked", which the destructive PATCH bypass had made false and which
externally edited Markdown cannot guarantee. The "What a plan does not do
yet" list is untouched, as T1.6 requires.

tasks.md: ticks the review-1 corrections (F1–F6, one commit each, shas
listed) under the review gate; T2.1/T2.2 shrink because `RevisePlan` — and
the Web field/milestone PATCH on it — shipped with the corrections, and note
the Phase 2 follow-up of folding `store.record_learning`'s validation into
the seam's copy once MCP `record_plan_learning` migrates.
…, six seats, arbitration

Why: the owner mandated that implementation, test results and documentation
are reviewed by a council including GPT Astra and Grok 4.6 plus a
best-for-purpose seat. This records both reviews and what was done about
each finding, so the gate decision is auditable.

Code review 1 (Phase 0 + 1 seam): GPT Astra REJECT on a real fourth door —
a compound PATCH composed a seam transition with a second unguarded save,
reproduced by hand (200/active/0 milestones/ready=false) before acceptance.
Six findings F1–F6 fixed at 705ba58b..f827f69c; verified 422/200/422/409 on
the probe, 4629 tests green. Grok's first run exhausted 16k tokens on
hidden reasoning (manifest.run1.json kept); the 40k re-run is the seat
weighed. qwen3-coder was the code seat.

§5 receipt review: unanimous that adopt:false is the correct reading of the
frozen rule, and that the historical +0.142 was mostly the crash fix main
already has. judge() now fails closed (crash reject, registered pair,
finite CI) with the frozen verdict re-derived byte-identical; ADR-0011
wording separates decision from execution. deepseek-r1 was the stats seat.

Gate: Phase 0+1 accepted as the Phase 2 base; §5 complete as measured.
The detect-secrets exclude now also covers the kept manifest.run1.json.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Integrate Study Plans across Now, Web architect, and MCP

1 participant