Skip to content

AM-1323 - Carry the plan into BUILD and GATE as a file, not a digest - #25

Merged
bretttully merged 11 commits into
mainfrom
AM-1323/plan-file-mcp-tool
Aug 7, 2026
Merged

AM-1323 - Carry the plan into BUILD and GATE as a file, not a digest#25
bretttully merged 11 commits into
mainfrom
AM-1323/plan-file-mcp-tool

Conversation

@bretttully

@bretttully bretttully commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Adds a plan_file MCP tool so /sy:ship's parent materialises the sole ACTIVE execution plan's ## For /sy:ship half to a file once per session and hands later phases its path plus a pin (comment_id, version). Nothing after the parent reads the tracker for the plan, and no phase loads plan text it does not need — the tool returns a path, never the body.

  • new tool plan_file (sy_tools/server.py), boundary derived from _AGENT_DETAIL_TAG, legacy separator still resolves, zero/several ACTIVE refused by count and comment id
  • /sy:ship contract prose: materialisation in § State router (every session), resume-only pin comparison routed as bail-to-spec, START no longer reads the ticket, BUILD and GATE named the plan file
  • ship-state.yaml gains plan_path/plan_comment_id/plan_version; all three read as null on an older file
  • nine new check_invariants assertions, each demonstrated failing against a broken copy
  • docs/smoke_mcp.py exercises the tool live via NON_VERB_SMOKED_TOOLS; REQUIRED_TOOLS and its count of 17 verb-serving tools are unchanged
  • the provenance header states the escape transformation as a condition rather than a fact, so it is true on a plain-Markdown tracker too (GitHub is passthrough — skills/tracker/github/ADAPTER.md:51)

Live read-back fidelity was proven read-only, not by the live smoke runSMOKE_LIVE=1 smoke_mcp.py run writes, and the only configured project is the production AM board. Its code is landed unchanged. Substituted at zero tracker writes: plan_file run against this ticket's own live ACTIVE plan comment 8891727. What that does and does not establish — including that recovered-equals-posted inside a single run stays offline-covered only — is in the acceptance-evidence comment below.

🤖 Generated with Claude Code

@bretttully

Copy link
Copy Markdown
Collaborator Author

Acceptance evidence: live read-back fidelity

SMOKE_LIVE=1 docs/smoke_mcp.py run was not executed. Do not read this as "live smoke passed." The only configured project is the production AM board, and that scenario writes — it creates an issue and posts comments. Its code (commit 623b959) is landed unchanged and will run as written the day it is pointed at a scratch project.

Substituted with the reviewer-checkable read-only proof below, at zero tracker writes — every live call is a GET. plan_file was run against this ticket's own live ACTIVE plan comment 8891727: a real two-part plan comment that Shipyard's own post-comment wrote to production Jira in an earlier session, so the write leg genuinely happened for real, just not inside this run.

Script live_proof.py, transcript logs/live-proof.log (both under scratch_dir("AM-1323")), RESULT: ALL PASS. Run against this worktree's code with CLAUDE_PROJECT_DIR pointed at the main checkout, so the code under proof is this branch's while the site and project resolve from the repository's own config layer.

Established live

  1. Selection and the pin. plan_file picks the sole ACTIVE plan out of a real live 2-comment thread — not a hand-built fixture — and returns exactly the five keys: comment_id 8891727, version 1, bytes 11999, comments_truncated false, and a path. No plan text in the result.
  2. Boundary detection against the body as Jira actually stores and returns it, through the real ADF pipeline. The boundary that matched is the current _AGENT_DETAIL_TAG collapsed-section form. The pre-509758d legacy separator is absent from this thread, so that fallback branch remains offline-tested only. The half on disk is byte-identical to _agent_half() applied independently to the live body.
  3. The documented escape transformation, observed on real live content. 3 converter-applied escapes in plain prose in comment 8891634 — un-backticked SHIP_WORKER_TRACKER_VERBS comes back as SHIP\_WORKER\_TRACKER\_VERBS. Two further raw \_ occurrences on the thread sit inside backticked spans in comment 8891727's own prose about the escaping; backticked spans come back verbatim, so those are author-written and are excluded, not counted as evidence (per-occurrence classification: logs/live-proof-escape-provenance.log). The recovered ACTIVE plan half itself has 0 converter-applied escapes, because its author backticked every identifier — a property of that content, not of the transformation.
  4. Stability / fixed point on the recovered half R: adf_to_markdown(markdown_to_adf(R)) equals R exactly but for one appended trailing newline, with zero interior difference (11630 B → 11631 B, R2 == R + "\n" true). A second round trip changes nothing further (R3 == R2), so it is idempotent from the first pass — this is what makes the transformation safe to document as a one-time, non-accumulating change. _agent_half strips, so plan_file's own output is exactly stable; two independent live runs produced a byte-identical plan-v1.md.
  5. Write containment. The tool's only write is plan-v1.md under scratch_dir("AM-1323") — snapshotted by content digest immediately either side of the one call. git status --porcelain is empty in both this worktree and the main checkout, before and after.

Not proven live, stated plainly

Recovered-equals-posted inside a single run. No pre-post source text for this plan exists anywhere locally, so a byte comparison of recovered vs. what was originally posted is unavailable and is not claimed here. That leg stays covered by:

  • sy_tools/tests/test_server.py's offline post-then-recover round-trip case;
  • sy_tools/tests/tracker/test_jira.py, which pins the read-back literals against the real converter rather than a fixture;
  • the landed-but-unexecuted live assertion in docs/smoke_mcp.py::Smoke._plan_file.

Also unexercised live by this run: the legacy-separator fallback, and the zero/several-ACTIVE refusals (both offline-tested).

…wn tracker

The header `plan_file` writes stated the rich-text escape transformation as
unconditional fact, while the tool's own docstring already worded it
conditionally. `skills/tracker/github/ADAPTER.md:51` is explicit that GitHub is
"Markdown passthrough — no conversion step", and `github/adapter.py`'s
`post_comment` hands the body to `gh` unconverted, so on a GitHub-backed tracker
every `plan-v{N}.md` carried a false claim about its own contents. Now states the
condition instead of asserting it, and is true either way.

Also drops a stray double space in test_jira.py's fixed-point assertion.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a new MCP tool (plan_file) to materialize the agent-facing half of the sole ACTIVE execution plan comment into an on-disk file and pass later /sy:ship phases a path + pin, reducing tracker reads and avoiding loading plan text in phases that don’t need it.

Changes:

  • Introduces plan_file in sy_tools/server.py, including plan selection (heading/status), boundary splitting (current + legacy), and file materialization.
  • Adds/updates tests and smoke coverage to pin “tracker read-back shape” and exercise plan_file end-to-end.
  • Updates /sy:ship and tracker contract docs, agents, and validation checks to use a plan file path/pin instead of a plan digest.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
sy_tools/server.py Adds plan_file, plan parsing, boundary extraction, and writes the plan half to a scratch file.
sy_tools/tests/test_server.py Adds fixtures and tests covering plan_file behavior and invariants.
sy_tools/tests/tracker/test_jira.py Pins Jira ADF↔Markdown read-back shapes used by plan_file fixtures.
skills/tracker/jira/ADAPTER.md Documents Jira comment read-back normalization/escaping relied on by plan_file.
skills/tracker/CONTRACT.md Updates “Exactly one ACTIVE plan” contract to note plan_file enforcement.
skills/spec/SKILL.md Adds guidance about backticking identifiers/paths/URLs to avoid read-back escapes.
skills/ship/SKILL.md Updates state router/invariants to materialize plan per session via plan_file.
skills/ship/references/start-resume.md Updates START contract/state fields to carry plan_path/plan_comment_id/plan_version.
skills/ship/references/implementation.md Updates BUILD guidance to read the plan from the plan file (no tracker read).
skills/ship/references/immutable-gate.md Updates GATE guidance to compose criteria from the plan file and pass its path.
skills/shared/references/context-economy.md Updates context guidance to reflect plan-half file materialization.
scripts/validate.py Adds validation assertions ensuring docs/agents mention the plan file and new state fields.
docs/smoke_mcp.py Smokes plan_file live (as a non-verb tool) without changing verb tool counts.
agents/ship-start.md Updates START agent contract to use plan file path/pin instead of reading tracker/digest.
agents/ship-build.md Updates BUILD agent contract to treat the plan file as authority for the plan.
.claude-plugin/plugin.json Bumps plugin version to 1.22.0.
Suppressed comments (1)

sy_tools/server.py:525

  • plan_file uses the raw issue string as the scratch directory identifier (config.scratch_dir(issue)). On the GitHub tracker, get_issue/create_issue return the issue URL as the id (see sy_tools/tracker/github/adapter.py:84-86), which includes : and /. That will create a deeply nested directory tree on POSIX and is likely to fail on Windows filesystems (invalid :), making plan_file unreliable across trackers/platforms.
    try:
        destination = config.scratch_dir(issue) / f"plan-v{version}.md"
    except config.ConfigError as exc:

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread sy_tools/server.py Outdated
bretttully and others added 3 commits August 7, 2026 14:58
…s, silent details-truncation, CRLF heading match, three vacuous validate.py checks, start-resume doc accuracy

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…y suffix not substring, disclose the smoke-script escape-shape gap, pin the validate.py scoping heading
…eck on the heading line itself (probe-verified), replace the unguarded _agent_half tests, dedupe smoke-script shape check

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@bretttully
bretttully marked this pull request as ready for review August 7, 2026 06:05
@bretttully
bretttully requested a review from Copilot August 7, 2026 06:05

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

Comment thread sy_tools/server.py
…eview

Matches the existing pattern in usage_summarize/export_transcript: a filesystem
failure now surfaces as a normal ToolError rather than an uncaught exception.
@bretttully

Copy link
Copy Markdown
Collaborator Author

GATE fix-cycle summary

Five sy:gate review passes against an immutable base (e14bcd9) with focused-delta re-review after each fix, converging at f98f81a:

  • Round 1 (full review, e14bcd9..02f1992): 2 HIGH + 6 MED/LOW findings, mostly in the unexecuted live-smoke path (docs/smoke_mcp.py's tracker-id/escape assumptions), _agent_half's boundary parsing, CRLF handling, and three vacuous scripts/validate.py assertions. Fixed in 61991c5.
  • Round 2 (61991c5..10e7003): the _agent_half fix from round 1 was itself incomplete (a nested <details> block could still defeat it) plus two smaller follow-ups. Fixed in 10e7003.
  • Round 3 (10e7003..e82e6e9): the validate.py heading-scoping fix was proven a no-op by mutation probe, and the new _agent_half tests didn't discriminate old-vs-new behaviour. Fixed in e82e6e9, this time probe-verified against three literal mutation attacks before being reported done.
  • Round 4 (e82e6e9): clean "safe to ship" verdict, independently re-derived rather than trusting the round-3 transcript. One _agent_half strictness edge case (a well-formed plan comment with trailing content after the outer </details> hard-refuses) was raised and deliberately deferred as a follow-up rather than spending a further round patching the same hand-rolled boundary parser a fourth time — it fails loudly, is unreachable via any path this repo's tooling writes today, and needs a real policy decision (accept trailing content vs. keep refusing) rather than a mechanical patch.
  • Round 5 (e82e6e9..f98f81a, narrow-fix cadence): after marking the PR ready and requesting Copilot, it flagged one real gap — plan_file's write_text had no OSError handling, unlike this module's other file-writing tools. Fixed directly (matches the in-file pattern verbatim) in f98f81a; confirmed safe to ship.

Disclosed, deferred, non-blocking — for a follow-up ticket, not this PR:

  1. _agent_half's exact-suffix close check refuses a plan comment with byte content trailing the outer </details> (e.g. a human note appended below the collapsed section). Loud failure, no wrong-plan selection, no path in this repo produces it today.
  2. Copilot's suppressed (non-blocking) comment on plan_file's scratch_dir(issue): on the GitHub adapter every issue id is the full issue URL, which would nest oddly and break on Windows (:). scratch_dir's containment check still holds (no escape), and this repo's tracker is Jira (config/defaults.json) with no GitHub-adapter path exercised live — but plan_file is the one call site in sy_tools/server.py that feeds a tracker-native id straight into scratch_dir rather than a caller-chosen key, so a real fix needs a design decision on deriving a safe cross-tracker scratch key, not a narrow patch.
  3. A sibling of the OSError fix just landed: config.scratch_dir's own mkdir can raise OSError, and both its call sites in server.py (plan_file and the scratch_dir MCP tool) currently catch only config.ConfigError, so that specific OSError still escapes raw. Same class as this round's fix, same low blast radius (loud, not silent).

A follow-up ticket will be filed at HANDOFF covering all three, plus the previously-known sy_tools/tracker/github/adapter.py ARTIFACT_LINK unanchored-regex gap noted during BUILD.

@bretttully

Copy link
Copy Markdown
Collaborator Author
Review coverage
REVIEW_BASE_SHA: e14bcd969d2ec58eff7567f1a2a005f5b8b402ca
REVIEWED_SHA: f98f81a19ba74273b76386e5bd020720ba817223
REVIEW_MODEL_REQUESTED: opus
REVIEW_EFFORT: max

Copilot review (PRRT_kwDOThwh386XMDXG): plan_file's success path could hand
out an empty comment_id when the sole ACTIVE comment's id reads back empty,
silently defeating the pin comparison a resume relies on for staleness
detection. The tool's own zero-or-many branch already treats a missing id as
"(no id)"; the success path now refuses the same condition instead of
returning an unusable pin.
…-6 nit

sy:gate (LOW, b67c81d): the refusal cross-referenced its own source position
("names above") which a ToolError recipient cannot see, and overstated the
consequence as making staleness detection wholly no-op when only one of the
pin's two moving fields (comment_id) is affected. Reworded to name the actual
fault (a tracker read fault, not a plan fault) and the action to take
(report it, don't repost the plan), matching the neighbouring refusals'
action-ending convention.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

Suppressed comments (1)

skills/shared/references/context-economy.md:26

  • The wording “Exactly one tracker comment is carried forward” is misleading now that later phases receive a file path (materialised from the plan comment) and no phase after the parent reads the ticket. Consider rephrasing to make it clear that it’s the plan comment’s ship half that’s carried forward as a file, not the comment itself.
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.

@bretttully
bretttully merged commit 645884c into main Aug 7, 2026
6 checks passed
@bretttully
bretttully deleted the AM-1323/plan-file-mcp-tool branch August 7, 2026 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants