The stage card shows what it is asking you to approve (#344, #267, #259) - #397
Open
hugolytics wants to merge 22 commits into
Open
The stage card shows what it is asking you to approve (#344, #267, #259)#397hugolytics wants to merge 22 commits into
hugolytics wants to merge 22 commits into
Conversation
…, #267, #259) Nine Block Kit variants in #ff-e2e settled the shape: a header block for the day, one section per group, Context and Decided folded into context blocks. Provenance names the rule and marks only what did not come from the user. The planner may ask one question below the artifact rather than instead of it. Reverses #267's flat-markdown payload: it cannot carry provenance the system can verify, and an unverified rule name on the card is the model-supplied-identifier failure #330 already found once. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Self-review dropped a task: the snapshot already carries applicable_constraints as flat rows with uid and name (#202), so the planned ACTIVE_CONSTRAINTS extension would have duplicated data the codebase already has. Spec corrected to say where the rules come from. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
applicable_constraints carries uid and name per row on every resolve (#202). The first draft of the plan would have built a parallel copy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The pre-flight scan found a test demanding behaviour Slack has no syntax for: * and _ cannot be escaped in mrkdwn. Aligns both card paths on html.escape(quote=False) and records what is deliberately not handled. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Provenance per item cannot live in flat markdown without the rule name being unverified model text (#267, #344). SkeletonItem/SkeletonGroup are new; SkeletonPayload now carries day_label/groups/reasoning instead of markdown/reasoning, refusing the old shape by name. The submit-gate refusal message, the planner obligation text in harness_bridge.py, and the two _known_field_names tuples that recognise these fields as system-minted all follow. stage_cards.py's skeleton branch does the minimum to keep drawing a card -- flattening groups/items to plain lines -- with a TODO for Task 4, which rewrites it to carry per-line provenance. Every existing test that built the old {"markdown": ...} shape is updated to the new one; none of them are weakened. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
762cea2's message overstated: it claimed every existing test building the old {"markdown": ...} skeleton payload was updated. It was not. Old-shape fixtures remained in test_adaptive_timeboxing.py, test_planner_can_ask_for_another_turn.py, test_timeboxing_intents.py, test_slack_timeboxing_channel_redirect.py, test_timeboxing_readiness.py, test_adaptive_stage1.py, test_harness_approval_action.py, and -- missed by the prior review pass too -- three recorded skeleton payloads inside tests/replay/fixtures/timeboxing_incident_20260829.json. None of these currently reach SkeletonPayload.model_validate, which is why the suite stayed green, but they teach a shape the contract now refuses, and Task 2's kernel-side verification on artifact acceptance would have hit several of them (the replay fixture in particular, which drives the real kernel) as confusing failures one task later. Every listed fixture now carries {day_label, groups, reasoning}: a minimal single-group, single source:"user" item where the test's assertions don't depend on the payload's content, or the original bullet text preserved verbatim as item text in the replay recording, where it does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Never act on a model-supplied identifier. #330 was a judge mistyping a uid by one character; here that would name a rule that does not exist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
open_questions was never a field the kernel read, so a planner with a question had nowhere to put it. A placement question is also unanswerable without the placement on screen, so it sits below the artifact. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…#259) Fix round 1, two Important findings: - blocking=False + a matching artifact + result.continuation is not None dropped the question with no guarantee the planner would re-raise it. NeedsAnotherTurn now carries an optional question instead of discarding it; refusing the combination was rejected because it would discard the turn's real work, the exact cost PlannerContinuation exists to avoid. - a riding question's option buttons could not be answered: the kernel never called _hold_question for it, and _release_question would have wiped pending_blocker on the same save regardless. Confirmed first that readiness and approval never read pending_blocker, then held it for both the riding and continuation-with-question branches and widened _release_question's exemption to match -- a plain approval or any other outcome still clears it, so a stale record self-heals once superseded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Style A, trailing and italic. Only what did not come from the user is marked, so every marker is something to argue with (#267). Reconciles the two Asking types: deletes stage_cards.Asking and repoints StageCard.asking at session_contracts.Asking, so AwaitingApproval.question rides onto the card with no conversion. _decided now returns facts only -- an assumption is marked inline on the artifact line it decided, not listed again in Decided. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ry card _decided(snapshot) gained include_assumptions=True by default, restoring DenyControl on stage 1/2/4 -- the global facts-only cut in the prior commit stripped deny from the stage-4 candidate approval, the last human gate before the calendar is written, with no inline marker to compensate. Only the SKELETON branch passes include_assumptions=False, where the inline _(my guess)_ marker already carries the same information. Restores the two tests that were rewritten to assert absence back to asserting presence for non-skeleton cards, adds a skeleton-scoped suppression test and a stage-4 DenyControl guard (#267). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A single long section collapses behind Slack's Show more, so the user clicked to see the day they were approving. A header block plus one section per group renders whole (#344). Decided items with no control (facts) fold into one uncapped context block; items with a control (an assumption's DenyControl) stay their own section with the overflow attached, since a context block cannot carry an accessory -- folding those too would re-break the Deny regression Task 4 just restored. Context and Decided both move to the end of the card, after asking/gate/nav, as supporting material rather than the thing being approved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…y once Review findings on #344 (2026-09-08): 1. _artifact_groups (stage_cards.py) is the sole escaper for a CardGroup -- one html.escape(x, quote=False) call covers both name and lines. render_stage_card was escaping group.name a second time, turning 'R&D' into 'R&amp;D'. It now renders the name verbatim, same as lines and body. The regression test that missed this constructed a raw CardGroup directly, bypassing the seam where the bug lived; replaced with a real-pipeline test (SkeletonPayload through _artifact_groups through render_stage_card) plus a pass-through test mirroring the one already covering card.body. 2. '*Decided*' now labels the section exactly once whenever card.decided is non-empty: above the controlled items when there are any, or on the folded context block only when every item folded. Previously an all-controlled Decided list (nothing left to fold) rendered as bare bullets with no heading. Block-budget arithmetic updated: 22 + N, N <= 18 (was 21 + N, N <= 19) -- the heading is one more block in the Decided worst case. Both fixes broken on purpose and confirmed to fail before restoring. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
_planning_obligation() said nothing about a blocker's `blocking` field: grep -c blocking on this file returned 0. A planner had no way to learn it may raise a non-blocking question that rides beside an artifact instead of replacing it, or that a second question in the same turn fails as too_many_questions regardless of blocking's value (#259 already paid for the silent version of this gap once, with an invented open_questions field the kernel never read). The paragraph is gated on skeleton and validated_candidate: those are the two targets _apply_planning_result actually reaches (per _derive_target), so they are the only turns that can end in AwaitingApproval with a riding question. day_frame and captured_inputs go through _planning_day_gate and _stage1_outcome instead and never see this branch. Drift guards: extended the SkeletonPayload field-name guard to SkeletonItem.model_fields (item fields appear as JSON-literal keys in payload_shape, not backtick references, so the check matches that form), and added one asserting the prompt names `blocking` for both gated targets. Left the .j2 template and prompt_rendering.py alone -- they are imported only by the legacy agent.py path slated for deletion (#192) and never produce a SkeletonPayload. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Task 6's prompt told the planner it could submit a non-blocking blocker beside an artifact, but two gaps beneath `submit_planning_result` made that unreachable in production: Gap 1 -- `_validated()` refused an artifact and any blocker together, unconditionally, regardless of `blocking`. Corrected to a case split: one non-blocking blocker rides with the artifact (the feature Task 3 built at the kernel); a *blocking* blocker beside an artifact is refused by name (`[blocking_blocker_with_artifact]`) because `_apply_planning_ result` checks `pending_question[1].blocking` before it ever looks at `artifact_updates` -- reaching the kernel would silently discard the artifact, not fail the turn. Two or more blockers beside an artifact are refused here too (`[too_many_questions]`), duplicated from the kernel the same way `required_block_missing` already is: the kernel's own refusal only fires once `_apply_planning_result` reaches the blocker branch, by which point this call has already returned `_RECORDED` and the planner believes the turn succeeded. Gap 2, the one that mattered most: `BlockerInput` never declared `blocking`, and neither it nor FastMCP's generated arg model sets `extra="forbid"`, so a `blocking: true` a planner actually sent over MCP was silently dropped before this tool's body -- and therefore before Gap 1's fix -- ever ran. Every existing test called the bare Python function with dicts, bypassing FastMCP's schema coercion entirely, so a green suite here proved nothing about the real wire. Added `blocking` to `BlockerInput`; confirmed via `mcp.list_tools()` that the generated schema now carries it, and via `mcp.call_tool()` (the real entry point) that it survives into the stored `UserBlockerDraft` and drives the correct refusal -- both traced in task-6b-report.md. Rewrote the tool docstring, which still said a blocker always replaces the artifact. Split `test_a_blocker_beside_an_artifact_is_refused` into three: non-blocking rides and succeeds, blocking is refused, two blockers is refused -- plus two wire-level tests using `mcp.call_tool` that would have caught Gap 2 on their own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Coordinator-directed addendum to the previous commit. `BlockerInput`, `AssumptionInput` and `BlockerOptionInput` set no `extra="forbid"`, so an undeclared field a planner sent over MCP was silently coerced away before it ever reached `_validated` -- the identical failure class as the `blocking`-drop just fixed, in the same three models, discovered in the same session. Re-read AssumptionInput's "loose on purpose" docstring before making this change: it says the strict contract lives in `_validated`, so this model's *types* don't have to be the gate (hence `value: Any`) -- it says nothing about tolerating an unknown field name, so `extra="forbid"` does not contradict it. Added `model_config = ConfigDict(extra="forbid")` to all three models -- this is an internal contract between one planner and one tool, not a versioned external API, so there is no forward-compatibility case for tolerating an unrecognised key. Amended the comment on `BlockerInput. blocking` to say the hazard is closed and why, so the next reader does not loosen it back. Verified over the real wire (`mcp.call_tool`, not the bare-dict path every other test in this file uses, which is what hid the original hazard): an unknown field on a blocker or an assumption is now refused with `pydantic.errors.extra_forbidden`, loudly, instead of disappearing. No existing caller or test sent an undeclared field, so nothing else in the suite was touched by tightening this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Task 4's _rule_names snippet read an ACTIVE_CONSTRAINTS fact -- leftover from a draft. That fact carries only a count; the implementation reads PlanningSessionSnapshot.applicable_constraints, the rows the host already resolved for the day (#202). Task 6's file target named skeleton_draft_system_prompt.j2 via render_skeleton_draft_system_prompt(), but that renderer is imported only by the legacy agent.py (slated for deletion, #192) whose Stage 3 never produces a SkeletonPayload; the live planner instruction actually changed was _planning_obligation() in harness_bridge.py. Both were controller overrides during execution and the plan document never caught up. Also appended a Task 6b section recording the added task: the MCP submission path refused an artifact and a riding blocker together unconditionally, and BlockerInput had no blocking field, so a planner's blocking:true was silently dropped before the tool body ran. Without both fixes the non-blocking-question feature built across Task 3 and Task 6 was unreachable in production. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ays asked Two ways the one non-blocking question could destroy something. The obligation invited a question on the candidate turn. It cannot be answered there: `evaluate` returns only the turn's target's gaps, and every user-owned requirement in the catalog targets SKELETON, so a blocker on a stage-4 turn names a gap that is `system` or `planner` owned and `_apply_planning_result` refuses the whole turn as `invalid_planner_result` -- discarding the finished candidate with it. The gate is skeleton-only, and says why at the gate so it is not widened again. A test asserts the kernel fact it rests on rather than leaving it to be re-derived. Re-presenting an unapproved skeleton dropped its riding question. The `_pending_approval` short-circuit answered with the artifact alone, `_release_question` saw no question and cleared the held record, the redrawn card had no question and the card that had one was receipted with its buttons stripped -- so a press on it refused as `stale_blocker_choice`, silently, on any Advance, NextControl or StartSession. `_still_asking` re-attaches it: the question text from the catalog (where `_asking` already reads it, so the same sentence), the options from the held record (a press binds against what was offered, never against what the catalog would offer now), and nothing at all once the requirement is satisfied. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Five small ones from the whole-branch review. `_ctx` sliced its join at Slack's block cap with no indication and both docstrings called it uncapped, so a long Decided list lost its tail and cut its last visible line mid-word -- on the card the user is being asked to approve. It now takes the items rather than the join, drops whole ones off the tail until they fit, and says how many went. The count cap stays retired; the claim of no cap at all does not. `unknown_rule_uid` reached the user as "I could not carry that through", and so did a stored skeleton in the retired shape -- where Retry is the one move that cannot work, because the next turn re-presents the same artifact and fails identically. Both have their own sentence now, and the undrawable one has its own code so the sentence can name Back and Proceed instead. `_known_rule_uids` admitted any row carrying a `uid` while `_rule_names` needs a string `name` to draw it with, so a row with one and not the other passed verification, was stored, and then raised inside `_artifact_groups` while drawing -- after the write. They read the rows the same way now. The stage-4 branch's missing `asking=` is now stated as the deliberate thing it is, and the skeleton validator's comment names the fields it actually checks rather than the `markdown` this branch retired. `test_a_non_blocking_question_keeps_proceed_and_a_blocking_one_does_not` asserted "Proceed" was absent from a card handed no `ApproveControl` -- true however the question renders. Replaced with the property it was reaching for: the question and its options are drawn, below the day, with Proceed still live. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Sep 8, 2026
Open
hugolytics
marked this pull request as ready for review
September 9, 2026 09:25
Resolves one conflict in harness_bridge._planning_obligation: both sides appended a fragment to the obligation string (question_channel here, work_lines on main). Both are kept. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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
2 participants
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.
Note: #359 (Stage 1 asks) merged into
mainat6762520on 2026-09-07, before this PRwas opened. This branch's rebase onto
origin/feat/stage1-elicitation-loopwas already ano-op (merge-base identical to
origin/main's), andgit merge-treeagainst currentmainproduces no conflicts. So despite the plan's "blocked by #359" framing, this branch is not
actually blocked and can merge directly into
main— flagging this since the plan and taskbrief were written before #359 merged.
Problem
Sunday 2026-09-06 committed 8 blocks at 00:04:37 and was corrected at 00:06:54. The card printed raw Markdown, buried the day under Decided, gave no sign that a sci-fi reading block came from a stored rule rather than from the user, and had nowhere to put the planner's open question.
Rubric
Human checklist
Rendered live: a real
SkeletonPayloadwas run throughmap_outcome/_artifact_groupsandrender_stage_card(the actual code path, no hand-written blocks) and posted to#ff-e2ethread
1788780130.551149(ts1788828740.625789), then read back to confirm: the day is aheaderblock, each group ("Morning"/"Afternoon"/"Evening") is its own section, only therule/assumed/calendaritems carry a marker (a real rule name, "my guess", "on yourcalendar" respectively) while the two
useritems carry none, the non-blocking question ridesbelow the artifact with Proceed/Back/Cancel still present, and the message came back whole —
11 blocks, no "Show more".
🤖 Generated with Claude Code