diff --git a/.claude-plugin/plugin.json b/.claude-plugin/plugin.json index 3c68ae0..803838b 100644 --- a/.claude-plugin/plugin.json +++ b/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "sy", "displayName": "Shipyard", - "version": "1.21.0", + "version": "1.22.0", "description": "Disciplined plan -> spec -> ship workflow for Claude Code: adversarial review against pinned commits, briefs-not-transcripts context hygiene, and a full paper trail on a pluggable issue tracker (Jira or GitHub Projects).", "author": { "name": "Brett Tully" diff --git a/agents/ship-build.md b/agents/ship-build.md index f43df5d..cf6ed5f 100644 --- a/agents/ship-build.md +++ b/agents/ship-build.md @@ -8,7 +8,7 @@ model: opus effort: high --- -You are the BUILD worker for `/sy:ship`. Follow `${CLAUDE_PLUGIN_ROOT}/skills/ship/references/implementation.md` exactly. Seeded with the state brief, standards contract, and plan anchors. Verify the plan's load-bearing plan facts before executing each step — re-locate cited anchors by content and re-check named conventions; a fact found false returns `needs-decision` (or `bail-to-spec` when it invalidates the contract), never gets followed. An open empirical question that a single spot-check left inconclusive, where continuing would move past what's already been pointed at, returns `needs-trace` with the question and its seed anchors — you never dispatch `sy:trace` yourself. Run the deterministic content-QA grep over doc/marketing deliverables before the draft PR, per the implementation reference. Delegate bounded slices to `sy:slice` and broad reconnaissance to `sy:sweep`; route verbose verification through logs in the task's resolved scratch directory (`scratch_dir($TASK_KEY)`). Never `Read` a raw image: delegate every visual inspection of figures/screenshots/plots to `sy:img-inspector` (model override: the model `agent_model {"name": "img-inspector"}` reports) and record its text verdicts as figure acceptance evidence, per `${CLAUDE_PLUGIN_ROOT}/skills/shared/references/image-inspection.md`. Resolve small deviations yourself and record them in `accepted_deviations`; never prompt the user. Keep the `phase_checkpoint` slice manifest current so every return is resumable. Resolve every subagent's model from config and pass it as the `Agent` invocation's actual model override, per `${CLAUDE_PLUGIN_ROOT}/skills/shared/references/model-dispatch.md` — including on nested dispatches, which inherit nothing. +You are the BUILD worker for `/sy:ship`. Follow `${CLAUDE_PLUGIN_ROOT}/skills/ship/references/implementation.md` exactly. Seeded with the state brief, the standards contract, and the plan file the state brief names — one file on disk carrying the plan's `## For /sy:ship` half verbatim, which is your authority for the plan rather than any tracker read you do not hold. Verify the plan's load-bearing plan facts before executing each step — re-locate cited anchors by content and re-check named conventions; a fact found false returns `needs-decision` (or `bail-to-spec` when it invalidates the contract), never gets followed. An open empirical question that a single spot-check left inconclusive, where continuing would move past what's already been pointed at, returns `needs-trace` with the question and its seed anchors — you never dispatch `sy:trace` yourself. Run the deterministic content-QA grep over doc/marketing deliverables before the draft PR, per the implementation reference. Delegate bounded slices to `sy:slice` and broad reconnaissance to `sy:sweep`; route verbose verification through logs in the task's resolved scratch directory (`scratch_dir($TASK_KEY)`). Never `Read` a raw image: delegate every visual inspection of figures/screenshots/plots to `sy:img-inspector` (model override: the model `agent_model {"name": "img-inspector"}` reports) and record its text verdicts as figure acceptance evidence, per `${CLAUDE_PLUGIN_ROOT}/skills/shared/references/image-inspection.md`. Resolve small deviations yourself and record them in `accepted_deviations`; never prompt the user. Keep the `phase_checkpoint` slice manifest current so every return is resumable. Resolve every subagent's model from config and pass it as the `Agent` invocation's actual model override, per `${CLAUDE_PLUGIN_ROOT}/skills/shared/references/model-dispatch.md` — including on nested dispatches, which inherit nothing. ## Return contract — target ≤800 tokens diff --git a/agents/ship-start.md b/agents/ship-start.md index a9b671a..9b27eb0 100644 --- a/agents/ship-start.md +++ b/agents/ship-start.md @@ -1,21 +1,21 @@ --- name: ship-start description: >- - START worker for /sy:ship: select the sole active plan, delegate standards resolution and - large Epic/plan reads, branch and worktree, seed resume state, and return the state brief. + START worker for /sy:ship: read the plan file the parent materialised, delegate standards + resolution, branch and worktree, seed resume state, and return the state brief. tools: Read, Write, Edit, Glob, Grep, Bash, Agent, Skill, mcp__plugin_sy_sy__get_config, mcp__sy__get_config, mcp__plugin_sy_sy__agent_model, mcp__sy__agent_model, mcp__plugin_sy_sy__scratch_dir, mcp__sy__scratch_dir, mcp__plugin_sy_sy__fingerprint_config, mcp__sy__fingerprint_config, mcp__plugin_sy_sy__memory_list, mcp__sy__memory_list, mcp__plugin_sy_sy__memory_search, mcp__sy__memory_search, mcp__plugin_sy_sy__set-status, mcp__sy__set-status, mcp__plugin_sy_sy__assign, mcp__sy__assign, mcp__plugin_sy_sy__check_env, mcp__sy__check_env model: sonnet effort: high --- -You are the START worker for `/sy:ship`. Follow `${CLAUDE_PLUGIN_ROOT}/skills/ship/references/start-resume.md` exactly. Seeded with the Task key, ship profile, and any prior state brief. Create the build worktree under the resolved `worktree.root` (`get_config {"key": "worktree.root"}`) per that reference. Delegate standards resolution and large Epic/plan tails so their raw reads stay out of your return; never prompt the user — surface decisions to the parent. Resolve every subagent's model from config and pass it as the `Agent` invocation's actual model override, per `${CLAUDE_PLUGIN_ROOT}/skills/shared/references/model-dispatch.md` — including on nested dispatches, which inherit nothing. +You are the START worker for `/sy:ship`. Follow `${CLAUDE_PLUGIN_ROOT}/skills/ship/references/start-resume.md` exactly. Seeded with the Task key, ship profile, any prior state brief, and the plan file's absolute path plus its pin — the parent materialised it with `plan_file` before dispatching you, and you hold no tracker read of your own. Create the build worktree under the resolved `worktree.root` (`get_config {"key": "worktree.root"}`) per that reference. Delegate standards resolution so its raw rule reads stay out of your return; never prompt the user — surface decisions to the parent. Resolve every subagent's model from config and pass it as the `Agent` invocation's actual model override, per `${CLAUDE_PLUGIN_ROOT}/skills/shared/references/model-dispatch.md` — including on nested dispatches, which inherit nothing. ## Return contract — target ≤700 tokens No preamble, narration, praise, pasted bodies, or tool recap. End with exactly one status block: ```text -DONE: fresh|resumed at ; BRANCH ; WORKTREE ; PLAN +DONE: fresh|resumed at ; BRANCH ; WORKTREE ; PLAN v @; FILE STATE: scratch_dir($TASK_KEY)/ship-state.yaml STANDARDS: MEMORY_REFUTE: none| diff --git a/docs/smoke_mcp.py b/docs/smoke_mcp.py index 00a3690..6eec9be 100644 --- a/docs/smoke_mcp.py +++ b/docs/smoke_mcp.py @@ -13,6 +13,9 @@ set-status, assign, link-parent, add-dependency, add-label, post-comment, post-log, link-pr, attach-artifact, type-convert, attachment-download, attachment-update. +Also exercised, serving no canonical verb: `plan_file` — posted a two-part plan comment and recovered +its agent-facing half off the live tracker, which is the only place that round trip is really proven. + Cleanup: created issues are left in place unless SMOKE_CLEANUP=1 moves them to `done`; the canonical verb surface has no delete verb, so removal is manual. Nothing this run did not create is touched. @@ -44,6 +47,33 @@ one would hide a typo in a real caller.""" STATUSES = ("backlog", "ready", "in-progress", "in-review", "done") +PLAN_HUMAN = "# Execution Plan v1\nStatus: ACTIVE\n\nTL;DR: smoke-test plan comment. Safe to delete." +"""The human half of a plan comment, in the shape `skills/spec/SKILL.md` §7 writes one.""" + +PLAN_AGENT_HALF = ( + "## For /sy:ship\n\n" + "Edit sy_tools/server.py and see [the contract](https://example.invalid/a_b) " + "before touching _AGENT_DETAIL_TAG.\n" +) +"""The agent-facing half `plan_file` has to recover. Deliberately carries un-backticked `_` and `[`. + +An escape-free half would read back byte-identical and the recovery assertion would prove nothing about +the transformation that actually happens to a real plan.""" + +PLAN_AGENT_ESCAPED = ( + "## For /sy:ship\n\n" + "Edit sy\\_tools/server.py and see [the contract]() " + "before touching \\_AGENT\\_DETAIL\\_TAG." +) +"""The same half as a rich-text tracker gives it back: un-backticked `_` escaped, the link target inside `<>`. + +Written out by hand rather than computed by calling the converter this script has no access to anyway. +The live check accepts this OR the untransformed `PLAN_AGENT_HALF` (a Markdown-passthrough tracker), +since this file names no tracker and cannot know upfront which applies — it reports which shape matched +instead of assuming one. That means a rich-text tracker whose escaping regressed to a no-op would read +as a legitimate Markdown-passthrough tracker rather than as a broken transformation; the transformation +itself stays pinned only by the offline converter tests in `sy_tools/tests/tracker/test_jira.py`.""" + VERB_TOOLS: dict[str, str] = { "validate_config": "validate_config", "preflight": "preflight", @@ -69,6 +99,19 @@ REQUIRED_TOOLS = frozenset(VERB_TOOLS.values()) +NON_VERB_SMOKED_TOOLS = frozenset({"plan_file"}) +"""Tools this scenario drives live that serve no canonical verb, so `VERB_TOOLS` cannot name them. + +Kept out of `REQUIRED_TOOLS`, whose length is the count of unique verb-serving tools: unioning one in +would corrupt that figure and the self-test that pins it. It is unioned in only where the question is +"is this tool present, and is it accounted for". + +`plan_file` is here rather than in `UNEXERCISED_TOOLS` because its whole contract is what a plan comment +looks like after a real tracker's rich-text round trip, which no fixture can stand in for. Its live call +does write a file — but under the *throwaway issue's* own scratch directory, keyed by an issue this run +created, so it touches nothing of the operator's the way a live `scratch_dir`, `memory_add` or +`reload_config` call would.""" + UNEXERCISED_TOOLS = frozenset({ "reload_config", "check_env", "get_config", "show_config", "agent_model", "scratch_dir", "fingerprint_config", "usage_summarize", "export_transcript", @@ -127,28 +170,36 @@ async def discover(self) -> None: """List the server's tools and report up front any the scenario needs and cannot find.""" listed = await self.client.list_tools() self.available = {tool.name for tool in listed.tools} - missing = sorted(REQUIRED_TOOLS - self.available) + missing = sorted((REQUIRED_TOOLS | NON_VERB_SMOKED_TOOLS) - self.available) print(f"==> server exposes {len(self.available)} tools") if missing: print(f"==> MISSING: {', '.join(missing)} — every verb needing one of these FAILs below") async def call(self, verb: str, args: dict[str, Any]) -> dict[str, Any] | None: """Invoke the tool serving `verb`, print its own error text on failure, return its payload.""" - tool = VERB_TOOLS[verb] + return await self.call_tool(verb, VERB_TOOLS[verb], args) + + async def call_tool(self, label: str, tool: str, args: dict[str, Any]) -> dict[str, Any] | None: + """Invoke `tool` and tally it under `label`; the path a non-verb tool is smoked through. + + Split out of `call` rather than adding `plan_file` to `VERB_TOOLS`: that map's values are what + `REQUIRED_TOOLS` and its pinned count are derived from, so a non-verb tool in it would report as + a nineteenth verb-serving tool. + """ if tool not in self.available: - self._fail(verb, f"the server exposes no tool named {tool!r}") + self._fail(label, f"the server exposes no tool named {tool!r}") return None result = await self.client.call_tool(tool, args) text = "".join(getattr(block, "text", "") for block in result.content) if result.is_error: - self._fail(verb, text or "the tool failed and said nothing") + self._fail(label, text or "the tool failed and said nothing") return None try: payload = json.loads(text) if text.strip() else {} except json.JSONDecodeError: - self._fail(verb, f"the tool returned a non-JSON success result: {text[:200]}") + self._fail(label, f"the tool returned a non-JSON success result: {text[:200]}") return None - self._pass(verb, tool) + self._pass(label, tool) return payload if isinstance(payload, dict) else {} def check(self, verb: str, ok: bool, detail: str) -> None: @@ -243,6 +294,65 @@ async def scenario(self, run_tag: str, tmp: Path) -> None: "agent_detail": PR_PLACEHOLDER, }) + await self._plan_file(second) + + async def _plan_file(self, issue: str) -> None: + """Post a real two-part plan comment, then prove `plan_file` recovers its agent-facing half. + + The only place the round trip is actually proven. `sy_tools/tests/test_server.py` asserts the + same equality against the converter called in-process, which is a fixture standing in for the + tracker; this asserts it against the tracker, which is the thing whose fidelity is in question — + a stored ADF document, a real REST read, and whatever normalisation the site does in between. + Proves whichever of the two known transformations applies, and reports which one matched; it does + not by itself distinguish a rich-text tracker's escaping regressing to a no-op from a genuine + Markdown-passthrough tracker — see `PLAN_AGENT_ESCAPED`'s docstring. + + On a *fresh* issue with no other plan on it, so the exactly-one-ACTIVE selection is unambiguous + without this run having to supersede anything. + """ + posted = await self.call("post-comment", { + "issue": issue, "human": PLAN_HUMAN, "agent_detail": PLAN_AGENT_HALF, + }) + if posted is None: + self.failures.append("plan_file: no plan comment to recover") + return + + payload = await self.call_tool("plan_file", "plan_file", {"issue": issue}) + if payload is None: + return + self.check( + "plan_file", + bool(payload.get("comment_id")) and payload.get("version") == 1, + f"the pin does not look right for the comment just posted: {payload} against {posted}", + ) + self.check( + "plan_file", + "For /sy:ship" not in json.dumps(payload), + f"plan text reached the tool result instead of staying on disk: {payload}", + ) + landed = Path(str(payload.get("path", ""))) + recovered = landed.read_text(encoding="utf-8") if landed.is_file() else "" + shape = ( + "escaped (rich-text tracker)" if recovered.rstrip().endswith(PLAN_AGENT_ESCAPED) + else "verbatim (Markdown-passthrough tracker)" if recovered.rstrip().endswith(PLAN_AGENT_HALF.rstrip()) + else "neither" + ) + print(f"==> plan_file escape shape: {shape}") + self.check( + "plan_file", + shape != "neither", + "the recovered half is not the posted half under either the rich-text escape transformation or " + "Markdown-passthrough.\n" + f" expected the file to end with either: {PLAN_AGENT_ESCAPED!r}\n" + f" or (Markdown-passthrough tracker): {PLAN_AGENT_HALF.rstrip()!r}\n" + f" file at {landed} holds: {recovered!r}", + ) + self.check( + "plan_file", + "TL;DR" not in recovered, + f"the human half leaked into the plan file: {recovered!r}", + ) + async def _attachments(self, run_tag: str, tmp: Path, issue: str) -> None: """Attach a scrubbed artifact, then round-trip it through download and update.""" artifact = tmp / f"{run_tag}-transcript.txt" @@ -358,12 +468,18 @@ def _self_test() -> None: registered = _registered_tools() assert registered, f"no @mcp.tool registration was found in {SERVER_SOURCE}; the scan is broken" - assert registered == REQUIRED_TOOLS | UNEXERCISED_TOOLS, ( + accounted = REQUIRED_TOOLS | NON_VERB_SMOKED_TOOLS | UNEXERCISED_TOOLS + assert registered == accounted, ( "the scenario and the server disagree about the tool surface. Only the scenario names: " - f"{sorted(REQUIRED_TOOLS - registered)}; only the server registers: " - f"{sorted(registered - REQUIRED_TOOLS - UNEXERCISED_TOOLS)}" + f"{sorted(accounted - registered)}; only the server registers: {sorted(registered - accounted)}" ) + # Unique verb-serving tools, so `NON_VERB_SMOKED_TOOLS` is deliberately not folded in: a tool that + # serves no verb is not a nineteenth verb-serving one, and unioning it here would corrupt the figure. assert len(REQUIRED_TOOLS) == 17, sorted(REQUIRED_TOOLS) + assert not (REQUIRED_TOOLS | UNEXERCISED_TOOLS) & NON_VERB_SMOKED_TOOLS, ( + "a non-verb smoked tool is also named as verb-serving or as unexercised; the three sets partition " + f"the surface: {sorted((REQUIRED_TOOLS | UNEXERCISED_TOOLS) & NON_VERB_SMOKED_TOOLS)}" + ) assert all(tool and tool == tool.strip() for tool in REQUIRED_TOOLS), sorted(REQUIRED_TOOLS) assert VERB_TOOLS["create-child"] == VERB_TOOLS["create-issue"], "a child is the create-issue write" assert VERB_TOOLS["link-pr"] == VERB_TOOLS["post-comment"], "a PR link's durable half is a comment" diff --git a/scripts/validate.py b/scripts/validate.py index e31724a..e5416c9 100755 --- a/scripts/validate.py +++ b/scripts/validate.py @@ -596,6 +596,8 @@ def numeric_tokens(target: str) -> set[str]: gate = read("agents/gate.md") spec = read("skills/spec/SKILL.md") start = read("skills/ship/references/start-resume.md") + ship_start_agent = read("agents/ship-start.md") + ship_build_agent = read("agents/ship-build.md") gate_ref = read("skills/ship/references/immutable-gate.md") contract = read("skills/tracker/CONTRACT.md") impl = read("skills/ship/references/implementation.md") @@ -647,13 +649,78 @@ def numeric_tokens(target: str) -> set[str]: "START cannot be checked before a later GATE dispatch", errors, ) - dispatch_fields = ("phase_active", "gate_rounds_total", "gate_rounds_budget_base") - if any(field not in start for field in dispatch_fields): + dispatch_fields = ( + "phase_active", + "gate_rounds_total", + "gate_rounds_budget_base", + # The plan pin joins the same check for the same reason: written once at START, read at resume. + "plan_path", + "plan_comment_id", + "plan_version", + ) + # Scoped to the state block itself, not the whole file: every one of these six is also discussed in the + # prose below it, so a file-wide check stays green on a field that fell out of the block a START run + # actually writes — which is the only place "stamped at START" is true or false. + start_state_block = start.partition("```yaml")[2].partition("```")[0] + if any(field not in start_state_block for field in dispatch_fields): + fail( + "ship start/resume's state block must stamp every per-dispatch field at START (the parent's own " + "phase_active, GATE's gate_rounds_total and gate_rounds_budget_base, and the plan pin the plan_file call " + "reported: plan_path, plan_comment_id, plan_version); a field never written at START is absent at " + "resume, leaving a session that died mid-phase, a spent fix-cycle round budget, or a plan revised " + "mid-run indistinguishable from a clean start", + errors, + ) + # Section-scoped: § Invariants names the tool too, and a file-wide check would stay green on that alone + # while § State router — the one place the per-session materialisation is procedure — quietly lost it. + ship_router = ship.partition("## State router")[2].partition("## Completion bar")[0] + if "calls `plan_file` on the Task" not in ship_router: + fail( + "ship SKILL's § State router must name plan_file: materialising the plan runs there, once per session " + "ahead of any dispatch, because a resume routing straight to BUILD or GATE passes through no phase " + "procedure that could own it and no worker holds a tracker read", + errors, + ) + heading_match = re.search(r"^## Resolve start model$", start, re.MULTILINE) + if heading_match is None: + fail( + "ship start/resume must carry a '## Resolve start model' heading (as its own line, not just " + "mentioned in prose); the plan_file mention check below is scoped to the procedure text before it, " + "and a missing or renamed heading leaves nothing correct to scope against", + errors, + ) + elif "plan_file" not in start[: heading_match.start()]: + fail( + "ship start/resume must name plan_file as where its plan comes from; without it, step 1 reads as a " + "tracker read this worker does not hold", + errors, + ) + if "vN digest" in ship_start_agent: + fail( + "agents/ship-start.md still returns the plan as a `vN digest`; START now returns the plan file's path " + "and pin, and a digest is not something a later phase can read a plan out of", + errors, + ) + if "PLAN v @; FILE " not in ship_start_agent: + fail( + "agents/ship-start.md must return the plan's pin (comment id + version) and file path in its DONE " + "line; a later phase resuming from this worker's return alone needs both to re-materialise or " + "compare against state, and a return that only names a digest gives it neither", + errors, + ) + for named, text in (("agents/ship-build.md", ship_build_agent), ("immutable-gate.md", gate_ref)): + if "the plan file the state brief names" not in text: + fail( + f"{named} must name the plan file the state brief names as how the plan reaches this phase; " + "neither BUILD nor GATE holds a tracker read, so a phase told to consult 'the plan' with no file " + "named has no way to", + errors, + ) + stale_economy_claim = "which is the only phase that reads the ticket" + if stale_economy_claim in economy_ref: fail( - "ship start/resume must stamp all three per-dispatch fields (the parent's own phase_active, plus GATE's " - "gate_rounds_total and gate_rounds_budget_base) at START; a field never written at START is absent at " - "resume, leaving a session that died mid-phase and a spent fix-cycle round budget indistinguishable " - "from a clean start", + f"context-economy.md still claims {stale_economy_claim!r}; no /sy:ship phase reads the ticket now — the " + "parent materialises the plan's ship half to a file and hands later phases its path", errors, ) if "pregate_checkpoint_channel" not in ship: diff --git a/skills/shared/references/context-economy.md b/skills/shared/references/context-economy.md index 83ca0d5..a7e6317 100644 --- a/skills/shared/references/context-economy.md +++ b/skills/shared/references/context-economy.md @@ -23,7 +23,7 @@ An artifact with two labeled parts — a human half and an agent half, a sign-of Forensic detail — the trace that established a fact, the counts, the hypotheses ruled out — is evidence for a claim, not an instruction to anybody. It earns a place inline only where a reader acts on it; otherwise it belongs in a companion record that nothing depends on. -In Shipyard that has a hard edge. A pointer to another tracker comment resolves to nothing for every phase after START, which is the only phase that reads the ticket. A fact a later phase needs is carried in that phase's own brief or it is not available at all — so "it's in the investigation comment" is not a way to keep detail without paying for it. Either the detail earns its place inline, or it goes in a companion comment the plan does not depend on. +In Shipyard that has a hard edge. Exactly one tracker comment is carried forward, and only half of it: the `/sy:ship` parent materialises the sole ACTIVE plan's own `## For /sy:ship` half to a file once per session and hands later phases that file's path (`${CLAUDE_PLUGIN_ROOT}/skills/ship/SKILL.md` § State router), and no `/sy:ship` phase reads the ticket itself. Every *other* comment still resolves to nothing for every phase — so a fact a later phase needs is in the plan's ship half, or in that phase's own dispatch brief, or it is not available at all, and "it's in the investigation comment" is not a way to keep detail without paying for it. Either the detail earns its place in the plan's ship half, or it goes in a companion comment the plan does not depend on. ## The instance already in the tree diff --git a/skills/ship/SKILL.md b/skills/ship/SKILL.md index 4f19cb4..1d45850 100644 --- a/skills/ship/SKILL.md +++ b/skills/ship/SKILL.md @@ -14,7 +14,7 @@ $ARGUMENTS ## Invariants - Before classifying state or dispatching any worker, the parent runs the tracker preflight (`${CLAUDE_PLUGIN_ROOT}/skills/shared/references/preflight.md`); a failure stops here with its single `## Action needed` block — no worker starts against an unusable tracker. -- Exactly one `# Execution Plan vN` is ACTIVE; otherwise stop for `/sy:spec`. +- Exactly one `# Execution Plan vN` is ACTIVE — enforced by the `plan_file` tool, which refuses zero or several by count and comment id rather than picking one; otherwise stop for `/sy:spec`. - Check plan-base freshness before building: material drift between `PLAN_BASE_SHA` and the ship base returns to `/sy:spec`. - Resolve standards before code. - The process tier (`full|light`) scales accounting records, never CI/review coverage. @@ -77,6 +77,10 @@ GATE itself never re-enters this section once dispatched, and this is now mechan Preflight (above) runs once, first, ahead of this classification — including on resume, since a checkpoint can route straight to BUILD or GATE without ever passing through START. +The plan is materialised in that same step, for the same reason, on every session alike — fresh and resume: the parent calls `plan_file` on the Task, which resolves the sole ACTIVE plan and writes that comment's `## For /sy:ship` half to a file, and the parent then names that file's absolute path in whichever worker's dispatch prompt it goes on to send. It belongs here rather than in any phase procedure because a resume routing straight to BUILD or GATE passes through none of them, and because no worker after the parent holds a tracker read at all — so a materialisation left to a phase would simply not happen on that path. The parent's own context never takes on the plan text: the tool returns a path and the pin, never the body. + +The pin comparison against recorded state is the resume-only half of that same call. On a fresh run there is nothing recorded to compare against and the returned `path`/`comment_id`/`version` are simply stamped into state as `plan_path`/`plan_comment_id`/`plan_version` (`references/start-resume.md`). On a resume the parent compares what `plan_file` just returned against what state already carries: unchanged, dispatch as the classification below decides; changed, the plan was revised mid-run and it is handled exactly as any `bail-to-spec` (§ Worker contract) — never resumed against a `phase_checkpoint` built from the plan that got superseded. State carrying none of the three fields is a file written before they existed and reads as `null`, which is nothing recorded rather than a mismatch: there is nothing to compare, and this step supplies them. + On resume the parent also loads `ship-state.yaml` here and drains it before dispatching whichever phase the classification lands on: any `memory_refutations` still listed are pending, not history, so the parent applies each via `memory_refute` and clears the list from state — the same drain rule as an in-flight worker return (§ Worker contract). It belongs in this pre-dispatch step because a resume routing straight to BUILD or GATE passes through no phase procedure that could own it, and the HANDOFF retro deliberately does not backstop it; an undrained list means the refuted anchor is still read back as if it held. `phase_active` is checked in that same pre-dispatch step and for the identical reason. A non-null value here means the last thing that happened was a dispatch whose return was never processed (§ Worker contract clears it on every return, including `blocked`), so the parent surfaces it in the state brief — the prior session ended without confirming its last dispatched phase finished cleanly, worth a quick sanity check of the recorded worktree before trusting the checkpoint — and then clears it to `null` unconditionally, whether or not the sanity check found anything, since the observation has already been made and a stale flag left set would report the same crash again after a clean resume. This changes nothing about classification: it is a note attached to whatever phase the classification lands on, never a reason to route somewhere else, and an absent or `null` field means simply that nothing was in flight. diff --git a/skills/ship/references/immutable-gate.md b/skills/ship/references/immutable-gate.md index fa3dbee..9a485f2 100644 --- a/skills/ship/references/immutable-gate.md +++ b/skills/ship/references/immutable-gate.md @@ -27,7 +27,7 @@ REVIEWED_SHA= TARGET_SHA= SHA at pin time> ``` -Create a detached review worktree, under the resolved worktree root (`get_config {"key": "worktree.root"}`; defaults to the sibling directory beside the repo), pinned to `REVIEWED_SHA`. Invoke `sy:gate` there with purpose, acceptance criteria, exact SHAs, standards authority, risk lenses, verification obligations, and the compact design contract (plan invariants plus `accepted_deviations` from state). Add `sy:gate` to `agents_used`. Gate verifies HEAD before reviewing. +Create a detached review worktree, under the resolved worktree root (`get_config {"key": "worktree.root"}`; defaults to the sibling directory beside the repo), pinned to `REVIEWED_SHA`. Invoke `sy:gate` there with purpose, exact SHAs, and the compact design contract (plan invariants plus `accepted_deviations` from state), composing the acceptance criteria, standards authority, risk lenses and verification obligations out of the plan file the state brief names — and passing that file's absolute path along with the brief, so the reviewer reads the plan's own words rather than this phase's paraphrase of them. That costs no tracker verb: the plan reached disk before GATE was ever dispatched (`${CLAUDE_PLUGIN_ROOT}/skills/ship/SKILL.md` § State router), `sy:gate` holds `Read`, and `hooks/review_guard.py` denies mutation inside the review worktree rather than reads outside it. Add `sy:gate` to `agents_used`. Gate verifies HEAD before reviewing. CI may run concurrently. Separate waiting from triage. Never poll `gh pr checks` or `gh run watch` once per reasoning turn, and never let a monitor self-resume at a turn-budget boundary — on a large matrix that bleeds tokens and the phase never returns. Wait with the single shared token-free background poller — launch `${CLAUDE_PLUGIN_ROOT}/scripts/ci_poll.sh poll ` with `run_in_background`; it sleeps between checks and exits when nothing is pending, spending no reasoning turns while it waits, and no phase hand-writes its own poller; only once CI is terminal, delegate the diagnosis to a `/sy:ci` subagent (added to `agents_used`) that returns a compact result rather than tailing raw logs. If CI cannot reach a terminal state within a sane bound (`ci.poll_timeout`, default 1800s — raise it for repos/matrices known to run long so one poll call spans the wait), return `blocked` (CI pending) with an idempotent checkpoint and the pending run id rather than looping. Never apply fixes to the review checkout. If code changes, finish/cancel stale review, fix in build worktree, push, and create a new immutable review scope. diff --git a/skills/ship/references/implementation.md b/skills/ship/references/implementation.md index dd3e58a..1850971 100644 --- a/skills/ship/references/implementation.md +++ b/skills/ship/references/implementation.md @@ -2,6 +2,8 @@ Follow ordered plan decisions. Resolve small details consistent with plan intent yourself and record them in `accepted_deviations`. A decision you cannot ground in plan/standards/code but that does not invalidate the plan returns `needs-decision` with an updated checkpoint; a new load-bearing fork or an invalidated contract returns `bail-to-spec`. An open empirical question is different from both: when a spot-check — or a delegate's own findings — comes back inconclusive and continuing would move past what's already been pointed at (a live external system, a scratch probe script, or a second follow-up command still chasing the same question), return `needs-trace` naming the open question and its seed anchors; the parent, never this worker, dispatches `sy:trace` and resumes you from the checkpoint with the findings. Never prompt the user. Use `sy:sweep` for broad reconnaissance. +Before the first slice, read the plan file the state brief names — the absolute path the parent's own `plan_file` call reported (`${CLAUDE_PLUGIN_ROOT}/skills/ship/SKILL.md` § State router), carrying the plan's `## For /sy:ship` half verbatim. That file is the authority for this phase's ordered changes, file anchors, verification obligations, design invariants, `docs requiring updates` and acceptance criteria; a dispatch prompt's summary of the plan is a pointer at it, never a substitute for it, and this worker holds no tracker read to go get the plan any other way. Read it whole, once, rather than per slice. + Before executing any plan step, verify its load-bearing plan facts against the current base: re-locate every cited file anchor by content (grep the surrounding phrase; never trust the plan's line numbers) and confirm each named convention still holds. A fact found false is never followed: a mismatch that leaves the plan's intent intact returns `needs-decision` with the mismatch and its bearing spans; one that invalidates the plan's contract returns `bail-to-spec`. An adjacent issue you surface mid-build that sits outside the plan's declared file set follows the same test: fold a small, low-risk fix into this branch as a recorded scope extension in `accepted_deviations` rather than filing a follow-up that loses the context you have now; defer only when it justifies its own ticket (see `${CLAUDE_PLUGIN_ROOT}/skills/shared/references/scope-discipline.md`). diff --git a/skills/ship/references/start-resume.md b/skills/ship/references/start-resume.md index 6881a7a..2942fbf 100644 --- a/skills/ship/references/start-resume.md +++ b/skills/ship/references/start-resume.md @@ -1,9 +1,9 @@ # Start or resume -This phase runs as the `sy:ship-start` worker: it initializes or resumes ownership, delegates standards resolution and large Epic/plan reads, and returns the state brief per the worker contract. +This phase runs as the `sy:ship-start` worker: it initializes or resumes ownership, delegates standards resolution, and returns the state brief per the worker contract. -1. Read Task body/comments and select the sole ACTIVE execution plan. -2. Read parent Epic only enough for sibling interfaces/blockers; use `sy:sweep` for a large tail. +1. The parent supplies the plan file's absolute path and its pin (`plan_comment_id`, `plan_version`) in this worker's dispatch prompt, having already materialised it with `plan_file` before dispatching anything (`${CLAUDE_PLUGIN_ROOT}/skills/ship/SKILL.md` § State router). Read `PLAN_BASE_SHA` and the `pre-gate checkpoint` channel out of that file. This worker holds no tracker read and never selects a plan itself — the selection, and the refusal when there is not exactly one ACTIVE, are the tool's. +2. Sibling interfaces and blockers off the parent Epic, when the parent's dispatch prompt states them, land here rather than via a fresh read: neither this worker nor `sy:sweep` holds a tracker read, so an instruction to go read the Epic here would be dead. Unlike `START_MODEL` (which `## Resolve start model` below pins as a genuine parent precondition), nothing currently obliges the parent to supply this — a dispatch that omits it simply leaves this worker with none, and that is not a resume failure. 3. Ship profile (the plan's explicit per-phase models, plus effort and process tier) is a parent precondition verified before dispatch; if the parent's own running session is below plan it stops and asks via `AskUserQuestion` (raise the profile / proceed at plan floor / other) per `${CLAUDE_PLUGIN_ROOT}/skills/shared/references/user-interaction.md`. That check concerns the parent's own session tier only; how each phase's model reaches its worker is the separate dispatch mechanism in `## Resolve start model` below. The profile floors worker models (may raise, never lower, so BUILD keeps its opus tier) and sets worker effort to match the work; it never lowers review effort (`sy:gate` stays max). Do not prompt the user from the worker. 4. Resolve standards in a delegate (subagent running `/sy:standards resolve `, added to `agents_used`) that returns only the retained contract — authority, implementation contract, primitives, risk lenses; rule-file reads stay out of the ship context. 5. Read durable cross-session memory — `memory_list` (or `memory_search` on the tools/surfaces the task touches) per `${CLAUDE_PLUGIN_ROOT}/skills/shared/references/memory.md`; a lesson that bears on the task enters the state brief as a known anchor. A known anchor this phase's own direct observation already contradicts is never carried forward as if it still held: author it as a `MEMORY_REFUTE` candidate in the return block and record it to `memory_refutations` in state, for the parent to apply — this worker holds no memory write itself. @@ -46,6 +46,9 @@ task: TASK-123 branch: task-123-example worktree: /abs/path plan_base_sha: +plan_path: +plan_comment_id: +plan_version: ship_base_sha: config_fingerprint: process_tier: full @@ -83,6 +86,6 @@ agents_used: [] Each phase's `*_model_requested` is written when that phase is dispatched and its `*_model_observed` only once the usage transcript confirms what ran, so the `build_model_*` pair stays `null` until the parent dispatches BUILD and the `review_model_*` pair stays `null` until GATE. `pregate_checkpoint_channel` is stamped once here too, taken from the plan's `pre-gate checkpoint` field in the normalized form the block above shows (`draft-pr` / `running-preview`) and left `null` when the plan declares none; `pregate_checkpoint_gate_dispatched` starts `false` and `pregate_checkpoint_request_text` starts `null` alongside it, stamped once here for the same reason the rest are; `${CLAUDE_PLUGIN_ROOT}/skills/ship/SKILL.md` § Pre-gate checkpoint owns how the rest are later set and re-checked. -The state file is local resume state, not shared truth. Never prune unrelated worktrees or paths. `phase_checkpoint` is the active worker's idempotent resume anchor (e.g. a slice manifest with per-slice status), passed to any continuation worker. Its neighbours are the run's dispatch bookkeeping. `phase_active` is the parent's own, never a worker's: it names the phase currently in flight and is set and cleared around every dispatch, so a value still set at resume means the prior session ended without confirming that phase finished (`${CLAUDE_PLUGIN_ROOT}/skills/ship/SKILL.md` § Worker contract and § State router own both halves). `gate_rounds_total` and `gate_rounds_budget_base` are GATE's live fix-cycle round accounting, owned by `references/immutable-gate.md` § Fix cycle — every pass this run has made, and the floor a raise-budget disposition stamps under it (`${CLAUDE_PLUGIN_ROOT}/skills/ship/SKILL.md` § Worker contract). All three are stamped once here so a resume reads them rather than inferring them, and an older state file carrying none of them reads as `null`/`0`/`0` respectively — nothing to detect, no rounds yet spent, no budget yet raised. +The state file is local resume state, not shared truth. Never prune unrelated worktrees or paths. `phase_checkpoint` is the active worker's idempotent resume anchor (e.g. a slice manifest with per-slice status), passed to any continuation worker. Its neighbours are the run's dispatch bookkeeping. `phase_active` is the parent's own, never a worker's: it names the phase currently in flight and is set and cleared around every dispatch, so a value still set at resume means the prior session ended without confirming that phase finished (`${CLAUDE_PLUGIN_ROOT}/skills/ship/SKILL.md` § Worker contract and § State router own both halves). `gate_rounds_total` and `gate_rounds_budget_base` are GATE's live fix-cycle round accounting, owned by `references/immutable-gate.md` § Fix cycle — every pass this run has made, and the floor a raise-budget disposition stamps under it (`${CLAUDE_PLUGIN_ROOT}/skills/ship/SKILL.md` § Worker contract). All three are stamped once here so a resume reads them rather than inferring them, and an older state file carrying none of them reads as `null`/`0`/`0` respectively — nothing to detect, no rounds yet spent, no budget yet raised. `plan_path`, `plan_comment_id` and `plan_version` are stamped here for the same reason, from what the parent's own `plan_file` call reported, and all three read as `null` on a state file written before they existed. A `null` pin is not a resume failure and needs no migration: the parent re-materialises the plan on every session ahead of any dispatch, so a `null` simply means there is nothing recorded yet for the fresh pin to be compared against, and the comparison is skipped rather than failed (`${CLAUDE_PLUGIN_ROOT}/skills/ship/SKILL.md` § State router). Return `done` with the state brief; the parent dispatches BUILD. diff --git a/skills/spec/SKILL.md b/skills/spec/SKILL.md index a81f019..b356db8 100644 --- a/skills/spec/SKILL.md +++ b/skills/spec/SKILL.md @@ -131,6 +131,8 @@ The plan itself has two clearly labeled parts, so a human reviewer and a fresh ` - tests and acceptance criteria; - plan base: `PLAN_BASE_SHA` of the inspected base. +Backtick every identifier, path and URL in the `/sy:ship` half that contains a `_` or a `*`: `/sy:ship` reads this half back off the tracker with `plan_file`, and a rich-text tracker escapes un-backticked Markdown punctuation on the way through, so a bare `some_name` reaches the builder as `some\_name`. + The docs-sync, visual-debug, and pre-gate-checkpoint fields are all required and all legitimately answerable with `none`; what counts for each, and what an honest answer looks like, is axes 4–6 of `${CLAUDE_PLUGIN_ROOT}/skills/shared/references/spec-gate.md` — read it there rather than from a copy here. An omitted field is what `sy:spec-gate` flags; a `none` on work that plainly touches a doc or a visual is what it flags harder; a `none` on the pre-gate checkpoint is the plan author's call, and is never second-guessed that way. End the `/sy:ship` part with `/sy:ship ` and a one-line ship profile that names every phase's model explicitly: `START / BUILD / GATE / effort / process `, such as `START opus / BUILD opus / GATE frontier / effort high / process full`. Naming the phases individually leaves `/sy:ship` nothing to infer — a single-word tier forced it to guess which phases the word applied to, and `/sy:ship` passes each stated model straight through as that phase's model override. diff --git a/skills/tracker/CONTRACT.md b/skills/tracker/CONTRACT.md index 5427a76..3366bd4 100644 --- a/skills/tracker/CONTRACT.md +++ b/skills/tracker/CONTRACT.md @@ -129,7 +129,7 @@ Generate usage from the on-disk transcript tree with the `usage_summarize` tool ## Exactly one ACTIVE plan -A `task`/`bug` carries at most one execution plan whose status is ACTIVE. Superseding is explicit: mark the old plan comment SUPERSEDED and the new one ACTIVE, then re-read to confirm exactly one ACTIVE. Never use a "latest-looking comment wins" heuristic. This is a core convention; the adapter only provides `post-comment`/`get-issue`. +A `task`/`bug` carries at most one execution plan whose status is ACTIVE. Superseding is explicit: mark the old plan comment SUPERSEDED and the new one ACTIVE, then re-read to confirm exactly one ACTIVE. Never use a "latest-looking comment wins" heuristic. This is a core convention; the adapter only provides `post-comment`/`get-issue`. For `/sy:ship` it is not left to the caller to uphold: the `plan_file` tool is what enforces it, refusing zero or several ACTIVE plans by count and comment id rather than picking one, so no run ships against a plan nobody approved. ## Configuration diff --git a/skills/tracker/jira/ADAPTER.md b/skills/tracker/jira/ADAPTER.md index ddba072..dabf2cd 100644 --- a/skills/tracker/jira/ADAPTER.md +++ b/skills/tracker/jira/ADAPTER.md @@ -52,7 +52,7 @@ Everything below is Jira-specific behaviour a caller can rely on. Where a verb i - **`create-issue`** creates into the configured project with the mapped native type. It deliberately does **not** send `reporter`, even though Jira's own `createmeta` lists it as required: omitting it makes Jira default the reporter to the authenticated account, where sending it would let a shared config file decide whose issues these are. Passing `parent` is the canonical verb **`create-child`**; Jira enforces its own hierarchy here, so a type that cannot be parented to the parent's type is rejected with the field named. - **`post-comment`** joins `human` and `agent_detail` into the one body, converts that Markdown to ADF, and returns the created comment's id and a deep link to it. `link-pr`'s durable half is this same verb — `human` noting that a PR now exists for this work, `agent_detail` carrying the PR URL — so Jira gets no separate write for it. `post-log` is its own verb whose assembled heading and fenced block convert to ADF the same way. A `shipyard.ship_metrics.v1` block is schema-validated before anything is posted; see `../CONTRACT.md`. The agent-facing half lands as a native Jira **Expand** section, collapsed by default with its caption as the title. The mechanism is narrow on purpose: `adf.py` rewrites the one fixed `
`/`` opening core emits into the `adf="expand"` attributed form `marklas` converts to an Expand node, matching that literal and nothing else. It is not a general `
` convention: an unrelated hand-authored disclosure block in some other body passes through the rewrite untouched, and `marklas` then discards its tags *and* its summary text, leaving only the enclosed content. Collapsing is a guarantee core's one caption earns, not a property of writing `
` into a Jira comment. -- **`get-issue`** reads REST directly and untruncated, naming the fields it needs rather than `*all` (which fetches every custom field on the board — kilobytes nothing above the seam reads). It returns the description as Markdown, canonical status and type, parent, children, `Blocks`-derived dependencies, and up to 50 comments newest-first, plus `comments_truncated` saying whether that bound actually cut anything off. A silently short thread reads as a complete ship log, so check that flag before concluding "no one raised this". The description read is not guaranteed byte-faithful to the stored ADF — unknown or extension nodes can round-trip through the Markdown conversion lossily — so a caller must never treat a `get-issue` read of the description as a faithful copy to merge, edit, or write back around other content; every `update-issue` description write is a full, deliberate replacement, never a conditional overwrite based on comparing against a prior read. Jira's `subtasks` field carries sub-task-level children only, so it is read for a known leaf type (Task, Bug) and nothing else: on any other type — Epic, Story, Initiative, a custom hierarchy level, or an issue whose `issuetype` is missing — `children` come from a `parent = ` search instead, because `subtasks` is empty on every one of them however decomposed they are. That search is scoped by `parent` alone (a key prefix is not a project — issues move, and hierarchies cross projects), and it is one page, so `children_truncated` says whether it left any child out. +- **`get-issue`** reads REST directly and untruncated, naming the fields it needs rather than `*all` (which fetches every custom field on the board — kilobytes nothing above the seam reads). It returns the description as Markdown, canonical status and type, parent, children, `Blocks`-derived dependencies, and up to 50 comments newest-first, plus `comments_truncated` saying whether that bound actually cut anything off. A silently short thread reads as a complete ship log, so check that flag before concluding "no one raised this". The description read is not guaranteed byte-faithful to the stored ADF — unknown or extension nodes can round-trip through the Markdown conversion lossily — so a caller must never treat a `get-issue` read of the description as a faithful copy to merge, edit, or write back around other content; every `update-issue` description write is a full, deliberate replacement, never a conditional overwrite based on comparing against a prior read. A **comment** read is that same caveat with a known, narrower shape, which `plan_file` depends on: structure survives — headings, lists, fenced blocks, and the one collapsed agent-facing section, which normalises back to the bare `
`/`` opening core wrote, so a later split on that boundary still finds it — while inline punctuation is transformed. Un-backticked Markdown punctuation comes back backslash-escaped (a bare `some_name` as `some\_name`), a link target comes back wrapped in `<>`, and one paragraph's several lines come back joined into one. Backticked spans and fenced blocks are verbatim. So a comment read is faithful enough to split on that boundary and hand the half on, and not faithful enough to diff against what was posted or to write back around. Jira's `subtasks` field carries sub-task-level children only, so it is read for a known leaf type (Task, Bug) and nothing else: on any other type — Epic, Story, Initiative, a custom hierarchy level, or an issue whose `issuetype` is missing — `children` come from a `parent = ` search instead, because `subtasks` is empty on every one of them however decomposed they are. That search is scoped by `parent` alone (a key prefix is not a project — issues move, and hierarchies cross projects), and it is one page, so `children_truncated` says whether it left any child out. - **`find-issues`** posts JQL to `/search/jql` (the classic `/search` endpoint is gone — 410). One page only: `is_last` and `next_page_token` are how a caller asks for more, and every interpolated value is a quoted JQL literal so a title containing a quote cannot widen the search. - **`add-dependency`** creates the `Blocks` link with `issue` as the blocked side and `blocked_by` as the blocker, taken straight from Jira's REST model, where the outward issue performs the type's outward action. It then re-reads to prove the direction took, and fails rather than warns if it cannot: a reversed dependency reads as entirely plausible and misleads every later decomposition. This is where the old `acli link --in/--out` recipe was wrong — those flags are inverted relative to Jira's model, so it silently created the reverse link. - **`add-label`** reads the current set and writes it back with the new label unioned in, because Jira has no append. A labels field that does not read back as a list of strings aborts the write instead of being coerced; coercing it would delete labels. diff --git a/sy_tools/server.py b/sy_tools/server.py index e0901e5..74bfb23 100644 --- a/sy_tools/server.py +++ b/sy_tools/server.py @@ -410,6 +410,146 @@ async def post_log( carrying it twice is a section nested in a section, and a tracker is free to render that by dropping the inner content — silently, and only in durable state — so the second one is refused at the door.""" +_LEGACY_AGENT_DETAIL_TAG = "\n\n---\n\n*Below this line: for a future agent session, not for your judgment.*\n\n" +"""The flat separator `post-comment` wrote before the collapsed section replaced it. + +Kept only so `plan_file` can still read a plan comment posted under it — a plan outlives the boundary +form it was written with, and a tracker read of one has to resolve rather than refuse. Nothing writes +it. Measured, not assumed: this literal survives the rich-text round trip byte for byte, so the read +path matches it as written.""" + +_PLAN_HEADING = re.compile(r"#[ \t]+Execution Plan v(\d+)[ \t]*\r?$", re.MULTILINE) +"""The heading an execution plan comment opens with, and the version number in it. + +Matched against the body's start, never searched for: a comment *quoting* a plan heading mid-body is +not a plan, and treating it as one is how "exactly one ACTIVE plan" starts picking the wrong comment.""" + +_PLAN_STATUS = re.compile(r"^Status:[ \t]*(\S+)", re.MULTILINE) +"""A plan's status field. The **first** occurrence after the heading is the plan's own status. + +First-match-wins rather than a body-wide search, and reading the value rather than testing containment: +a SUPERSEDED plan whose prose later says the word ACTIVE must not read as active, and a plan's own field +is the line the convention in `skills/spec/SKILL.md` §7 writes directly under the heading. The status +line is not required to still be a line of its own by the time it is read back — the rich-text round +trip merges it into one paragraph with a following `Supersedes:` — which is why the value is taken with +`\\S+` off the field name rather than off a whole-line match.""" + + +def _agent_half(body: str) -> str: + """The agent-facing half of a two-part comment body, under whichever boundary form wrote it.""" + if _AGENT_DETAIL_TAG in body: + half = body.split(_AGENT_DETAIL_TAG, 1)[1] + # Cut the close by suffix, never by searching for `
`: the half may legitimately carry its own + # nested block holding that exact literal, so only a true trailing suffix is the real outer close. + if half.endswith(_AGENT_DETAIL_CLOSE): + return half[: -len(_AGENT_DETAIL_CLOSE)].strip() + if "
" not in half: + return half.strip() + raise ToolError( + "the ACTIVE plan comment's agent-facing section carries a `
` that is not the outer " + "close `post-comment` appends: cutting at it would risk dropping content after a nested " + "disclosure block instead of at the section's real end. Repost the plan through `post-comment` " + "so the outer close lands last." + ) + if _LEGACY_AGENT_DETAIL_TAG in body: + return body.split(_LEGACY_AGENT_DETAIL_TAG, 1)[1].strip() + raise ToolError( + "the ACTIVE plan comment carries neither boundary this tool can split on: neither the collapsed " + "agent-facing section `post-comment` writes nor the flat separator that preceded it. It was not " + "posted as a two-part comment, so it has no `## For /sy:ship` half to hand a later phase; repost " + "the plan through `post-comment` with the two parts passed separately." + ) + + +@mcp.tool(name="plan_file") +async def plan_file(issue: IssueId) -> dict[str, Any]: + """Write the sole ACTIVE execution plan's agent-facing half to a file, and report where it landed. + + The way a `/sy:ship` phase after START gets the plan. It reads the issue, selects the one comment + that opens `# Execution Plan v` and carries `Status: ACTIVE`, and materialises that comment's + agent-facing half — the `## For /sy:ship` part — under the issue's own scratch directory. The plan + text is never part of the result: a caller gets a path plus the pin (`comment_id`, `version`) and + hands both on, so no phase loads plan text it does not need and a plan revised between sessions is + detectable by comparing the pin rather than by re-reading prose. + + Zero or more than one ACTIVE plan is refused, naming the count and the comment ids: that convention + (`skills/tracker/CONTRACT.md` § Exactly one ACTIVE plan) is what makes "the plan" unambiguous, and + picking one of several would ship against a plan nobody approved. `comments_truncated` says whether + the issue's comment page left anything out — on zero found, the ACTIVE plan may simply be past the + newest page rather than absent. + + What lands on disk is the plan half **as the tracker gives it back**, not as it was posted: a + rich-text tracker escapes un-backticked Markdown punctuation on the way through, so `some_name` + written without backticks reads back as `some\\_name` and a link target arrives inside `<>`. A + two-line header the tool writes records the version, the comment id and that transformation, so a + reader of the file knows which text is authoritative. Nothing is scrubbed on this path: it is a read + whose output stays on the machine that ran it, and a silent redaction inside a file a later phase + treats as the plan would be a change to the plan with no signal that it happened. + """ + _required(issue=issue) + read = await tracker.adapter().get_issue(issue) + # `.get` with a default, not `[...]`: only one adapter reports the flag, and an adapter that cannot + # tell must read as "nothing known to be cut off" rather than making this tool unusable there. + truncated = bool(read.get("comments_truncated", False)) + active: list[tuple[str, int, str]] = [] + for comment in read.get("comments") or []: + if not isinstance(comment, dict): + continue + body = str(comment.get("body") or "").lstrip() + heading = _PLAN_HEADING.match(body) + if heading is None: + continue + status = _PLAN_STATUS.search(body, heading.end()) + if status is None or status.group(1) != "ACTIVE": + continue + active.append((str(comment.get("id") or ""), int(heading.group(1)), body)) + if len(active) != 1: + ids = ", ".join(f"{comment_id or '(no id)'} (v{version})" for comment_id, version, _ in active) + raise ToolError( + f"{issue} has {len(active)} ACTIVE execution plans, not one" + + (f": {ids}. " if ids else ". ") + + ( + "The ACTIVE plan may also be past the newest page of comments this read returned " + "(comments_truncated is true), so treat it as unresolved rather than absent. " + if not active and truncated + else "" + ) + + "Exactly one plan is ACTIVE by convention and superseding is explicit; resolve it in " + "/sy:spec rather than picking one here." + ) + comment_id, version, body = active[0] + if not comment_id: + raise ToolError( + f"{issue}'s sole ACTIVE execution plan (v{version}) has no readable comment id: the tracker " + "read back an empty id for its own comment, so the pin this tool returns could not reliably " + "detect a later revision at the same version. This is a tracker read fault, not a plan fault; " + "report it rather than reposting the plan." + ) + text = ( + f"\n" + "\n\n" + + _agent_half(body) + + "\n" + ) + try: + destination = config.scratch_dir(issue) / f"plan-v{version}.md" + except config.ConfigError as exc: + raise ToolError(str(exc)) from None + try: + destination.write_text(text, encoding="utf-8") + except OSError as exc: + raise ToolError(f"plan file could not be written to {destination}: {exc}") from None + return { + "path": str(destination), + "comment_id": comment_id, + "version": version, + "bytes": len(text.encode("utf-8")), + "comments_truncated": truncated, + } + # Loose on purpose — markers are interchangeable and the counts need not match: looseness can only ever # *find* a block, and a block found is a block validated, where a block missed reaches the tracker unread. diff --git a/sy_tools/tests/test_server.py b/sy_tools/tests/test_server.py index cdb4672..058d15c 100644 --- a/sy_tools/tests/test_server.py +++ b/sy_tools/tests/test_server.py @@ -42,6 +42,7 @@ "memory_list", "memory_refute", "memory_search", + "plan_file", "post-comment", "post-log", "preflight", @@ -102,8 +103,19 @@ class _Recorder: # write test below would compare a function to an int in the body-size check and raise TypeError. body_limit = 32_767 - def __init__(self) -> None: + def __init__(self, issue: dict | None = None) -> None: self.calls: list[tuple[str, tuple, dict]] = [] + self.issue = issue + + async def get_issue(self, issue: str) -> dict: + """A real method, not `__getattr__`'s stub: `plan_file` reads the thread this returns. + + Still recorded, so the wiring assertion above keeps covering `get-issue` unchanged. `issue` + unset falls back to that stub's answer rather than to an empty thread, which would make a + `plan_file` test pass for having found no plan when the fixture simply was not wired. + """ + self.calls.append(("get_issue", (issue,), {})) + return self.issue if self.issue is not None else {"verb": "get_issue"} def __getattr__(self, verb: str): async def record(*args: Any, **kwargs: Any) -> dict: @@ -1687,3 +1699,226 @@ async def test_the_transcript_tools_take_exactly_one_source(tool, arguments): result = await client.call_tool(tool, arguments) assert result.is_error is True, result.content assert "not both and not neither" in _text(result), _text(result) + + +# ---- plan_file -------------------------------------------------------------------------------- +# +# Every fixture body below is in the shape a *rich-text* tracker gives a comment back, not the shape it +# was posted in, because that is the only shape this tool ever sees: escapes on un-backticked +# punctuation, a link target wrapped in `<>`, one paragraph's lines joined. The halves carry an +# un-backticked `_` and `[` on purpose — an escape-free fixture would let a recovery assertion pass +# while proving nothing about what a real plan comment reads back as. +# +# That the read-back literals here really are what the converter produces from the posted ones is +# pinned in `sy_tools/tests/tracker/test_jira.py`, which is the zone allowed to name a concrete +# tracker (`sy_tools/tests/test_tracker_seam.py`), and proven against the live tracker itself in +# `docs/smoke_mcp.py`. This file asserts the tool's own behaviour over those shapes. + +PLAN_HUMAN = "# Execution Plan v3\nStatus: ACTIVE\nSupersedes: v2\n\nTL;DR: ship the plan-file tool." +PLAN_AGENT = ( + "## For /sy:ship\n\n" + "Edit sy_tools/server.py and see [the contract](https://example.invalid/a_b) " + "before touching _AGENT_DETAIL_TAG.\n" +) +"""The two halves as `/sy:spec` posts them. Kept here so the read-back literals below have a source.""" + +PLAN_HUMAN_READ_BACK = "# Execution Plan v3\n\nStatus: ACTIVE Supersedes: v2\n\nTL;DR: ship the plan-file tool." +PLAN_AGENT_READ_BACK = ( + "## For /sy:ship\n\n" + "Edit sy\\_tools/server.py and see [the contract]() " + "before touching \\_AGENT\\_DETAIL\\_TAG." +) +"""The same two halves as the tracker gives them back — note the merged status line and the escapes.""" + +SUPERSEDED_READ_BACK = "# Execution Plan v2\n\nStatus: SUPERSEDED Superseded by: v3\n\nStatus: ACTIVE is prose here." +"""A superseded plan whose prose says the words a body-wide containment check would match.""" + + +def _comment_body(human: str, agent: str, *, boundary: str | None = None) -> str: + """One two-part comment body, assembled around the boundary `post-comment` writes.""" + if boundary is not None: + return human + boundary + agent + return human + server._AGENT_DETAIL_OPEN + agent + server._AGENT_DETAIL_CLOSE + + +def _plan_comment(*, boundary: str | None = None) -> str: + """The ACTIVE plan comment as read back, in whichever boundary form wrote it.""" + return _comment_body(PLAN_HUMAN_READ_BACK, PLAN_AGENT_READ_BACK, boundary=boundary) + + +def _thread(*bodies: str, truncated: bool = False) -> dict: + """A `get_issue` payload whose comments carry `bodies` as the tracker gave them back.""" + comments = [ + {"id": f"c{index}", "author": "a", "created": "2026-01-01", "body": body} + for index, body in enumerate(bodies, start=1) + ] + return {"id": "PROJ-1", "comments": comments, "comments_truncated": truncated} + + +@pytest.fixture +def scratch_root(tmp_path, monkeypatch) -> Path: + """Point the resolver's scratch root at a throwaway directory, leaving the real one alone. + + `config.get` is patched rather than `config.scratch_dir`, so the containment and creation logic + under test is the shipped one; any other key read on this path fails the test rather than silently + resolving against the operator's own configuration. + """ + root = tmp_path / "scratch" + root.mkdir() + + def only_scratch(key: str, *_a: Any, **_k: Any) -> str: + assert key == "scratch.dir", f"plan_file read config key {key!r}, which this fixture does not stub" + return str(root) + + monkeypatch.setattr(server.config, "get", only_scratch) + return root + + +@pytest.mark.anyio +async def test_plan_file_writes_the_recovered_agent_half_and_returns_a_pointer_never_the_text( + monkeypatch, scratch_root +): + """The whole contract in one call: the right half on disk, a path and a pin back, no plan text.""" + recorder = _Recorder(_thread(SUPERSEDED_READ_BACK, _plan_comment())) + monkeypatch.setattr(server.tracker, "adapter", lambda: recorder) + async with mcp.Client(server.mcp) as client: + result = await client.call_tool("plan_file", {"issue": "PROJ-1"}) + assert result.is_error is False, result.content + + written = (scratch_root / "PROJ-1" / "plan-v3.md").read_text(encoding="utf-8") + expected = PLAN_AGENT_READ_BACK + assert "\\_" in expected, "the fixture must carry an escape, or this asserts nothing about the read back" + assert written.endswith(expected + "\n"), f"the recovered half is not what the tracker gives back: {written!r}" + assert "TL;DR" not in written, f"the human half leaked into the plan file: {written!r}" + header = written[: written.index(expected)] + assert len(header.strip().splitlines()) == 2, f"the provenance header must be two lines: {header!r}" + for pin in ("v3", "c2", "PROJ-1", "\\_"): + assert pin in header, f"the provenance header must name {pin!r}: {header!r}" + + payload = _payload(result) + assert set(payload) == {"path", "comment_id", "version", "bytes", "comments_truncated"}, payload + assert payload["path"] == str(scratch_root / "PROJ-1" / "plan-v3.md"), payload + assert (payload["comment_id"], payload["version"]) == ("c2", 3), payload + assert payload["bytes"] == len(written.encode("utf-8")), payload + assert payload["comments_truncated"] is False, payload + assert "For /sy:ship" not in str(result), f"plan text reached the tool result: {result}" + + +@pytest.mark.anyio +async def test_plan_file_resolves_a_plan_posted_under_the_separator_that_preceded_the_section( + monkeypatch, scratch_root +): + """A plan outlives the boundary form it was written with, so the older one still has to resolve.""" + recorder = _Recorder(_thread(_plan_comment(boundary=server._LEGACY_AGENT_DETAIL_TAG))) + monkeypatch.setattr(server.tracker, "adapter", lambda: recorder) + async with mcp.Client(server.mcp) as client: + result = await client.call_tool("plan_file", {"issue": "PROJ-1"}) + assert result.is_error is False, _text(result) + written = Path(_payload(result)["path"]).read_text(encoding="utf-8") + assert written.rstrip().endswith(PLAN_AGENT_READ_BACK), written + assert "TL;DR" not in written, f"the human half leaked past the legacy separator: {written!r}" + + +NESTED_HALF = "before\n\n
\n\nnested\n\ninner\n\n
\n\nafter" +"""An agent half carrying its own disclosure block, whose close is byte-identical to the outer one.""" + + +def test_a_nested_block_with_no_outer_close_is_refused_not_silently_truncated(): + """A close-shaped literal that is not the suffix once cut the half short at it, losing everything after.""" + body = server._AGENT_DETAIL_OPEN + NESTED_HALF + with pytest.raises(server.ToolError, match="not the outer"): + server._agent_half(body) + + +def test_a_nested_block_under_a_real_outer_close_keeps_everything_up_to_that_close(): + """Cutting at the nested close instead of the outer one would drop the half's trailing content.""" + half = server._agent_half(server._AGENT_DETAIL_OPEN + NESTED_HALF + server._AGENT_DETAIL_CLOSE) + assert half == NESTED_HALF, f"the half was cut at the nested block instead of the outer close: {half!r}" + + +def test_the_legacy_separator_returns_a_nested_shaped_half_whole(): + """The flat separator never carried a close, so close handling must not reach it and refuse a valid plan.""" + half = server._agent_half(server._LEGACY_AGENT_DETAIL_TAG + NESTED_HALF) + assert half == NESTED_HALF, f"the legacy half was cut at its own disclosure block: {half!r}" + + +@pytest.mark.anyio +@pytest.mark.parametrize( + ("case", "bodies", "truncated", "expected"), + [ + ("none", (SUPERSEDED_READ_BACK,), False, "has 0 ACTIVE execution plans"), + ("none-truncated", (), True, "comments_truncated is true"), + ("two", (_plan_comment(), _plan_comment()), False, "c1 (v3), c2 (v3)"), + ], + ids=["none", "none-past-the-page", "two"], +) +async def test_plan_file_refuses_anything_but_exactly_one_active_plan( + monkeypatch, scratch_root, case, bodies, truncated, expected +): + """Picking one of several would ship against a plan nobody approved, so both counts refuse loudly.""" + recorder = _Recorder(_thread(*bodies, truncated=truncated)) + monkeypatch.setattr(server.tracker, "adapter", lambda: recorder) + async with mcp.Client(server.mcp) as client: + result = await client.call_tool("plan_file", {"issue": "PROJ-1"}) + assert result.is_error is True, result.content + assert expected in _text(result), _text(result) + assert not list(scratch_root.rglob("plan-v*.md")), "a refusal must not leave a plan file behind" + + +@pytest.mark.anyio +async def test_plan_file_reports_a_pin_that_a_revised_plan_moves(monkeypatch, scratch_root): + """A plan superseded between sessions has to be detectable, never silently read as the current one. + + The comparison itself is the `/sy:ship` parent's — `skills/ship/SKILL.md` § State router routes a + changed pin exactly as a `bail-to-spec`. What the tool owes that comparison is a pin that actually + moves: a return naming only the path, or one that reported the superseded comment because it came + first in the thread, would leave a resume unable to tell a revision from a re-read. + """ + v3 = _plan_comment() + v4 = _comment_body( + PLAN_HUMAN_READ_BACK.replace("v3", "v4").replace("Supersedes: v2", "Supersedes: v3"), + PLAN_AGENT_READ_BACK, + ) + threads = ( + _thread(SUPERSEDED_READ_BACK, v3), + _thread(SUPERSEDED_READ_BACK, v3.replace("Status: ACTIVE", "Status: SUPERSEDED"), v4), + ) + pins = [] + for thread in threads: + monkeypatch.setattr(server.tracker, "adapter", lambda bound=thread: _Recorder(bound)) + async with mcp.Client(server.mcp) as client: + result = await client.call_tool("plan_file", {"issue": "PROJ-1"}) + assert result.is_error is False, _text(result) + payload = _payload(result) + pins.append((payload["comment_id"], payload["version"], payload["path"])) + assert pins[0] == ("c2", 3, str(scratch_root / "PROJ-1" / "plan-v3.md")), pins + assert pins[1] == ("c3", 4, str(scratch_root / "PROJ-1" / "plan-v4.md")), ( + f"the second read must report the revised plan's own pin, not the superseded one: {pins}" + ) + assert pins[0] != pins[1], "a revised plan must not read back as the pin already recorded in state" + + +@pytest.mark.anyio +async def test_plan_file_refuses_an_active_plan_whose_comment_id_reads_back_empty(monkeypatch, scratch_root): + """An empty comment_id would hand out an unusable pin, silently defeating the staleness comparison.""" + thread = _thread(_plan_comment()) + thread["comments"][0]["id"] = "" + recorder = _Recorder(thread) + monkeypatch.setattr(server.tracker, "adapter", lambda: recorder) + async with mcp.Client(server.mcp) as client: + result = await client.call_tool("plan_file", {"issue": "PROJ-1"}) + assert result.is_error is True, result.content + assert "no readable comment id" in _text(result), _text(result) + assert not list(scratch_root.rglob("plan-v*.md")), "a refusal must not leave a plan file behind" + + +@pytest.mark.anyio +async def test_plan_file_refuses_a_plan_carrying_neither_boundary(monkeypatch, scratch_root): + """A one-blob plan comment has no agent-facing half to hand on, and must not yield the whole body.""" + recorder = _Recorder(_thread(PLAN_HUMAN_READ_BACK + "\n\n" + PLAN_AGENT_READ_BACK)) + monkeypatch.setattr(server.tracker, "adapter", lambda: recorder) + async with mcp.Client(server.mcp) as client: + result = await client.call_tool("plan_file", {"issue": "PROJ-1"}) + assert result.is_error is True, result.content + assert "neither boundary" in _text(result), _text(result) + assert not list(scratch_root.rglob("plan-v*.md")), "a refusal must not leave a plan file behind" diff --git a/sy_tools/tests/tracker/test_jira.py b/sy_tools/tests/tracker/test_jira.py index 996bf57..39a5baf 100644 --- a/sy_tools/tests/tracker/test_jira.py +++ b/sy_tools/tests/tracker/test_jira.py @@ -16,7 +16,14 @@ import httpx2 import pytest -from sy_tools.server import _AGENT_DETAIL_CLOSE, _AGENT_DETAIL_OPEN +from sy_tools.server import _AGENT_DETAIL_CLOSE, _AGENT_DETAIL_OPEN, _agent_half +from sy_tools.tests.test_server import ( + PLAN_AGENT, + PLAN_AGENT_READ_BACK, + PLAN_HUMAN, + PLAN_HUMAN_READ_BACK, + SUPERSEDED_READ_BACK, +) from sy_tools.tracker import TIMEOUT_SECONDS, TrackerError from sy_tools.tracker.jira import adapter, adf @@ -1795,3 +1802,40 @@ def test_the_collapsed_section_survives_a_read_and_a_write_back(): expands = _expands(adf.markdown_to_adf(read_back)) assert len(expands) == 1, f"the section flattened on its way back through: {read_back!r}" assert expands[0]["attrs"]["title"] == EXPAND_TITLE, f"the expand lost its caption: {read_back!r}" + # The same normalisation is what `plan_file` splits a read-back plan comment on, so the boundary + # surviving here and the extractor still finding it are one fact, asserted in one place. + assert _agent_half(read_back) == "HEAD 6144373", ( + f"the agent half is no longer recoverable from a read-back body: {read_back!r}" + ) + + +def test_a_plan_comment_reads_back_as_the_shape_the_core_tests_fixture_on(): + """`plan_file`'s own tests fixture on hand-written read-back literals; this is what pins them. + + They cannot compute the shape themselves: `sy_tools/tests/test_tracker_seam.py` keeps every concrete + tracker — this converter included — inside `sy_tools/tracker/` and its own tests, and a core test + importing it would be the seam breach that rule exists to catch. So the literals live beside the + tool and their truth is asserted here, where naming the converter is legal. A conversion change that + alters the escaping fails here, naming the new shape, instead of leaving those fixtures quietly + describing a transformation that no longer happens. + + Both halves, not just the agent one: the human half's read-back shape is what the version and status + selection is applied to, and its status line stops being a line of its own on the way through. + """ + posted = PLAN_HUMAN.strip() + _AGENT_DETAIL_OPEN + PLAN_AGENT.strip() + _AGENT_DETAIL_CLOSE + read_back = adf.adf_to_markdown(adf.markdown_to_adf(posted)) + assert read_back.startswith(PLAN_HUMAN_READ_BACK), ( + f"the human half no longer reads back as the core fixture says:\n fixture: {PLAN_HUMAN_READ_BACK!r}\n" + f" actual: {read_back!r}" + ) + assert _agent_half(read_back) == PLAN_AGENT_READ_BACK, ( + f"the agent half no longer reads back as the core fixture says:\n fixture: {PLAN_AGENT_READ_BACK!r}\n" + f" actual: {_agent_half(read_back)!r}" + ) + assert "_" in PLAN_AGENT and "\\_" in PLAN_AGENT_READ_BACK, ( + "the fixture must carry an un-backticked underscore and its escaped form, or this pins nothing" + ) + assert adf.adf_to_markdown(adf.markdown_to_adf(SUPERSEDED_READ_BACK)).strip() == SUPERSEDED_READ_BACK, ( + "the superseded-plan fixture must already be at the conversion's fixed point, or it is not a " + "read-back shape at all" + )