diff --git a/.claude-plugin/plugin.json b/.claude-plugin/plugin.json index 395c4a6..3c68ae0 100644 --- a/.claude-plugin/plugin.json +++ b/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "sy", "displayName": "Shipyard", - "version": "1.20.0", + "version": "1.21.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/CONTRIBUTING.md b/CONTRIBUTING.md index d3bb574..847dd69 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -11,7 +11,7 @@ Contributions are small, verifiable, and keep the tracker seam clean. This guide 4. Load the plugin locally to try it: `claude --plugin-dir /path/to/shipyard`. 5. Commands are namespaced by the plugin name (`sy`): `/sy:plan`, `/sy:spec`, `/sy:ship`, `/sy:spike`, `/sy:pr`, `/sy:ci`, `/sy:explain`, `/sy:help`, `/sy:init-repo`, `/sy:config`. -Keep prose (READMEs, roadmaps, docs) clear and unwrapped; keep machine-facing text (agent briefs, contracts, JSON logs) terse and structured. +Keep prose (READMEs, roadmaps, docs) clear and unwrapped; keep machine-facing text (agent briefs, contracts, JSON logs) terse and structured. How hard to cut either, and the two tests to cut by, is `skills/shared/references/context-economy.md`. ## Comments and docstrings diff --git a/docs/usage.md b/docs/usage.md index 822e77d..411a93d 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -38,7 +38,7 @@ Re-enter later with `/sy:plan ` to read what shipped since the last checkp `` is an issue ID — a Jira key like `PROJ-123` or a GitHub issue like `#123`. `/sy:spec` reads the ticket and the code, resolves the repo's engineering standards, pulls representative data when shape matters, and asks you only when research cannot settle a decision. It writes a complete plan — the approach, the strongest rejected alternative (pressure-tested by the same debate, which runs on every plan before sign-off), ordered changes with file anchors, tests, acceptance criteria, a verification obligation for every activated risk lens, and the docs, visual, and pre-gate-checkpoint obligations the change makes necessary — then has a separate `sy:spec-gate` reviewer read that drafted plan for architecture, simplicity, correctness, and those last three fields before you ever see it. -You approve the plan before anything is built, and what you are asked to approve is a short prose summary of the judgment calls rather than the full file inventory. On approval the mechanical plan lands on the ticket as the single ACTIVE execution plan, stamped with the commit it was planned against. That same prose summary is posted as a comment on the ticket — the body is left untouched, so a reporter's repro steps or a PM's acceptance notes are never a target of the write. The task moves to `ready`, and the plan ends with a `/sy:ship` kickoff and a ship profile that names each phase's model explicitly (`START / BUILD / GATE / effort / process `), so `/sy:ship` passes each phase's model through as an actual override instead of inferring one. +You approve the plan before anything is built, and what you are asked to approve is a short prose summary of the judgment calls rather than the full file inventory. On approval the mechanical plan lands on the ticket as the single ACTIVE execution plan, stamped with the commit it was planned against. That plan comment is the only thing written — the ticket body is left untouched, so a reporter's repro steps or a PM's acceptance notes are never a target of the write, and the prose summary you just approved is not posted back as a second comment restating it. The task moves to `ready`, and the plan ends with a `/sy:ship` kickoff and a ship profile that names each phase's model explicitly (`START / BUILD / GATE / effort / process `), so `/sy:ship` passes each phase's model through as an actual override instead of inferring one. Not every spec ends in a plan. When research shows the premise is already delivered, invalidated, or superseded, spec shelves the task instead: it posts the decisive evidence as a comment and closes the task rather than producing a plan for work that should not ship. diff --git a/scripts/validate.py b/scripts/validate.py index 1ac7b0f..e31724a 100755 --- a/scripts/validate.py +++ b/scripts/validate.py @@ -64,6 +64,9 @@ # Every agent must be able to ask whether a credential is present without ever reading its value; # `sy_tools/guards/secret_guard.py` names this tool as the remedy it steers shell probes toward. CHECK_ENV_TOOL = "check_env" +# The lowest number an adapter's `body_limit` could plausibly be. Both real limits are 32k and 64k, and +# no tracker documents anything near this floor, so a declaration under it is a typo, not a limit. +BODY_LIMIT_FLOOR = 8_192 _SCRATCH_HINT = "the `sy` server's `scratch_dir` tool" _SCRATCH_REF_SUFFIXES = {".md", ".py", ".sh", ".json", ".yml", ".yaml", ".toml"} _SCRATCH_REF_PATTERN = re.compile(r"(? None: def read(rel: str) -> str: return (ROOT / rel).read_text(encoding="utf-8") + # Read out of the source rather than imported: `python scripts/validate.py` puts `scripts/` on + # `sys.path`, not the repo root, so importing the adapters needs a `sys.path` shim this file has + # never needed for anything else. The comment block directly above the declaration comes back with + # it, because that block is a copy of the figure too and the same anchor already locates it. + # `None`, not a sentinel figure: a `0` here fell through into the floor leg and reported a second, + # fabricated fault ("this is a dropped digit") about a declaration that was never read. + def declared_body_limit(rel: str) -> tuple[int, str] | None: + match = re.search(r"((?:^ *#.*\n)*)^ {4}body_limit: int = ([\d_]+)$", read(rel), re.MULTILINE) + if match is None: + fail(f"{rel} must declare `body_limit: int = `; its ADAPTER.md figure cannot be checked", errors) + return None + return int(match.group(2)), match.group(1) + + # Every grouped or ungrouped spelling of a four-digit-or-longer number in `target`. Used instead of + # substring containment, which a target satisfies for the wrong reason whenever the declared digits + # happen to appear inside some unrelated number in its prose. + # + # The boundaries are deliberately asymmetric: the lookbehind rejects a leading hyphen so a citation + # id cannot be read as a figure, but the lookahead rejects a trailing hyphen only when a digit + # follows it. Making them symmetric loses `32,767-character`, which is how prose actually writes the + # figure, and a target stating only that reads as figure-free — so the staleness leg passes over + # stale provenance and the whole check goes quietly vacuous. + def numeric_tokens(target: str) -> set[str]: + return set(re.findall(r"(? str: preflight_ref = read("skills/shared/references/preflight.md") debate_ref = read("skills/shared/references/debate.md") spec_gate_ref = read("skills/shared/references/spec-gate.md") + economy_ref = read("skills/shared/references/context-economy.md") + contributing = read("CONTRIBUTING.md") roadmap_shaping = read("skills/plan/references/roadmap-shaping.md") tracker_skill = read("skills/tracker/SKILL.md") jira_adapter = read("skills/tracker/jira/ADAPTER.md") @@ -812,6 +843,88 @@ def read(rel: str) -> str: fail("spec §7's Step 2 procedure must state it never writes the Task body", errors) if "Step 2 — after approval" not in spec: fail("spec must keep the staged reveal: full plan posted only in Step 2, after approval", errors) + # §7 Step 2's summary comment was removed: it restated, on the ticket, a summary the user had just + # read and approved. Both spellings are pinned because the instruction lived in two places — Step 2's + # numbered procedure and Step 1's auto-mode consent sentence — and a consent sentence still naming a + # write the run no longer performs states a false authorization. + if "post the Step-1 summary" in spec_s7: + fail("spec §7 must not post the Step-1 summary back as a second comment restating the plan", errors) + if "post this summary as a comment on the Task" in spec: + fail("spec's sign-off consent sentence names a summary-comment write §7 Step 2 no longer performs", errors) + + # Context economy is a single copy: the two cut tests are phrased once, in the reference, and every + # authoring surface carries a pointer to it. A consumer that spells a cut test out has forked the rule. + cut_tests = ( + "Does removing this sentence change what its reader does?", + "Would a pointer do the work this text is doing?", + ) + for cut_test in cut_tests: + if cut_test not in economy_ref: + fail(f"context-economy.md must state the cut test {cut_test!r} verbatim", errors) + economy_consumers = ( + ("spec", spec), + ("plan", plan), + ("pr", pr), + ("handoff-accounting", handoff), + ("CONTRIBUTING.md", contributing), + ) + for name, text in economy_consumers: + if "context-economy.md" not in text: + fail(f"{name} authors an agent-facing artifact and must cite context-economy.md", errors) + for cut_test in cut_tests: + if cut_test in text: + fail(f"{name} restates a context-economy cut test; cite context-economy.md instead", errors) + economy_axis_phrase = "narrates what a cited anchor already shows" + if economy_axis_phrase not in spec_gate_ref: + fail(f"spec-gate reference's Simplicity axis must state the prose trigger {economy_axis_phrase!r}", errors) + if economy_axis_phrase in spec_gate: + fail("spec-gate agent restates the prose-economy trigger; cite spec-gate.md instead of copying it", errors) + if economy_axis_phrase in spec: + fail("spec restates the prose-economy trigger; cite spec-gate.md instead of copying it", errors) + + # Each adapter's body limit lives in the constant, again in the comment above it carrying that + # figure's provenance, and again in its agent-facing ADAPTER.md prose. The Protocol docstring is no + # longer a target: it states the contract and names no figure, because a core module may not name a + # concrete tracker (CONTRIBUTING.md) and a figure is worthless without its tracker-specific + # provenance. Both spellings count everywhere, because the prose groups thousands and the code does not. + for name, source, doc_rel, doc in ( + ("jira", "sy_tools/tracker/jira/adapter.py", "skills/tracker/jira/ADAPTER.md", jira_adapter), + ("github", "sy_tools/tracker/github/adapter.py", "skills/tracker/github/ADAPTER.md", github_adapter), + ): + declared = declared_body_limit(source) + if declared is None: + continue + limit, note = declared + spellings = {str(limit), f"{limit:,}", f"{limit:_}"} + # A dropped leading digit used to survive substring containment by colliding with the grouped + # spelling it came from — `2_767` occurs inside every target stating `32,767` — cutting the limit + # tenfold with every doc stale. Whole-token matching closes that, and a floor closes the rest of + # the class without parsing prose: no tracker limit is anywhere near this low. + if limit < BODY_LIMIT_FLOOR: + fail( + f"{name} adapter's body_limit is {limit} ({source}), under the {BODY_LIMIT_FLOOR} floor; no " + "tracker limit is that low, so this is a dropped digit, not a limit", + errors, + ) + if not spellings & numeric_tokens(doc): + fail(f"{name} adapter's body_limit is {limit} ({source}); {doc_rel} states no such figure", errors) + # The comment above the declaration is the copy the doc leg never reads, so it is checked for two + # faults. Detachment: the anchor only reaches a block sitting immediately above the declaration, so + # one blank line between them emptied `note` and left this leg passing on a stale figure. Staleness: + # every figure the block does state is the declared one. + if not note.strip(): + fail( + f"{name} adapter's body_limit is {limit} ({source}) with no comment directly above it; the " + "provenance comment must sit on the lines immediately preceding the declaration", + errors, + ) + stale = sorted(numeric_tokens(note) - spellings) + if stale: + fail( + f"{name} adapter's body_limit is {limit} ({source}); the comment above the declaration " + f"still states {stale[0]}", + errors, + ) # `post-comment` takes `human` and `agent_detail`, both required, and assembles the boundary itself. # These are the highest-traffic call sites, so a reference still describing one hand-composed body diff --git a/skills/plan/SKILL.md b/skills/plan/SKILL.md index 7ef3d63..f2ec9d4 100644 --- a/skills/plan/SKILL.md +++ b/skills/plan/SKILL.md @@ -36,7 +36,7 @@ Read durable cross-session memory early — `memory_list` (or `memory_search` on Seed every agent prompt with known anchors — paths, symbols, entry points, keys — and name ground already covered; agents must not rediscover what the caller knows. -Machine-facing agent briefs stay pointer-dense. Human-facing Epic maps and decision logs remain clear prose. +Machine-facing agent briefs stay pointer-dense. Human-facing Epic maps and decision logs remain clear prose. Both, and the roadmap and `# SEAMS` comments this skill posts, are written under `${CLAUDE_PLUGIN_ROOT}/skills/shared/references/context-economy.md`. ## State router diff --git a/skills/pr/SKILL.md b/skills/pr/SKILL.md index 7568cf2..367e3a9 100644 --- a/skills/pr/SKILL.md +++ b/skills/pr/SKILL.md @@ -36,6 +36,8 @@ Base on real diff/log. The mutable description is the PR's human-attention secti **Acceptance criteria/evidence never live only in the mutable description.** `/sy:ship` posts them as a dedicated PR comment so promotion/refresh cannot erase them. +What belongs in which of those two, and how hard to cut each, is `${CLAUDE_PLUGIN_ROOT}/skills/shared/references/context-economy.md`. + Title: ` - ` when branch carries a ticket key. ## 3. Review threads diff --git a/skills/shared/references/context-economy.md b/skills/shared/references/context-economy.md new file mode 100644 index 0000000..83ca0d5 --- /dev/null +++ b/skills/shared/references/context-economy.md @@ -0,0 +1,30 @@ +# Context economy + +Every agent-facing artifact Shipyard writes is loaded into a finite budget and read by a model that gets worse at using it the fuller it gets. Anthropic's guidance names the resource directly — "LLMs have an 'attention budget' that they draw on when parsing large volumes of context" — and sets the target as finding "the smallest possible set of high-signal tokens that maximize the likelihood of some desired outcome" (https://www.anthropic.com/engineering/effective-context-engineering-for-ai-agents). This file is the single copy of that rule for Shipyard's own artifacts: skills, agent briefs, plans, roadmaps, PR descriptions, state briefs, handoff records. Cite it from wherever an artifact gets authored; never restate it there. + +The cost is measured, not stylistic. Chroma's *Context Rot* evaluated 18 LLMs across 194,480 calls and found that "even under these minimal conditions, model performance degrades as input length increases…" — on tasks trivial enough that length was the only variable (https://www.trychroma.com/research/context-rot). Position compounds volume: performance "is often highest when relevant information occurs at the beginning or end of the input context, and significantly degrades when models must access relevant information in the middle of long contexts, even for explicitly long-context models" (Liu et al., https://arxiv.org/abs/2307.03172). And instructions dilute each other — IFScale reports that "even the best frontier models only achieve 68% accuracy at the max density of 500 instructions", with later instructions dropped more often than earlier ones (https://arxiv.org/abs/2507.11538). A sentence added to a brief is not free: it competes with every other sentence there, and it pushes something else toward the middle. + +## The two cut tests + +Apply both to every paragraph before it ships: + +1. **Does removing this sentence change what its reader does?** If nothing downstream changes, it is commentary, and it goes. +2. **Would a pointer do the work this text is doing?** If the content already lives somewhere the reader can reach, cite that instead of copying it. + +## Write to the actor + +A settled decision is stated, not re-argued. Once the choice is made, the artifact tells its reader what to do; the reasoning that produced it belongs to the human-facing half of the record, or nowhere. Rationale in machine-facing text is the most common form of dilution here precisely because it reads as thoroughness — but an implementer cannot act on *why*, and every line of it displaces a line they could have acted on. + +## No cross-part restatement + +An artifact with two labeled parts — a human half and an agent half, a sign-off summary and a mechanical plan — carries each fact in exactly one of them. Repetition across that boundary is not redundancy for safety; it is two copies that drift, and a reader of either half cannot tell which one is current. Cross-reference across the boundary instead of copying over it. + +## Evidence is not instruction + +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. + +## The instance already in the tree + +Shipyard applies this to itself: the `## Return contract — target ≤N tokens` block in every `agents/*.md` is this principle made enforceable at the one point where an agent's output enters someone else's budget. diff --git a/skills/shared/references/spec-gate.md b/skills/shared/references/spec-gate.md index a10068f..8623370 100644 --- a/skills/shared/references/spec-gate.md +++ b/skills/shared/references/spec-gate.md @@ -11,7 +11,7 @@ The checklist below is the single copy. Cite this file from the dispatch prompt Three are reviewer judgment — the reviewer has to think, and a pass with nothing found is a real result: 1. **Architecture.** Does the change sit at the right altitude and on the right seam? Look for a new parallel path beside an existing one, a primitive reimplemented instead of reused, a responsibility landing in a layer that should not own it, and coupling the plan introduces but never names. -2. **Simplicity.** Is this the smallest change that delivers the goal? Look for a config toggle where an unconditional behaviour would do, a new abstraction with one caller, ordered steps that collapse into one, and scope the goal does not require. +2. **Simplicity.** Is this the smallest change that delivers the goal? Look for a config toggle where an unconditional behaviour would do, a new abstraction with one caller, ordered steps that collapse into one, and scope the goal does not require. The plan's own prose is in scope, on two objective triggers: prose restating a decision argued elsewhere in the same plan, and prose that narrates what a cited anchor already shows (`${CLAUDE_PLUGIN_ROOT}/skills/shared/references/context-economy.md`). Neither is a length budget — a long plan whose every paragraph is load-bearing passes this axis. 3. **Correctness.** Do the ordered changes actually produce the stated outcome? Look for a step whose stated effect its cited anchor cannot have, an invariant the plan breaks elsewhere while protecting it here, an ordering that leaves an intermediate state broken, and an acceptance criterion that would pass without the behaviour existing. Three are required-field completeness — the plan's `/sy:ship` section either carries the field or it does not: diff --git a/skills/ship/references/handoff-accounting.md b/skills/ship/references/handoff-accounting.md index 6b026f4..f56c897 100644 --- a/skills/ship/references/handoff-accounting.md +++ b/skills/ship/references/handoff-accounting.md @@ -2,7 +2,7 @@ This phase runs mostly as a worker for the records and accounting. The readable transcript is rendered from the on-disk session tree by a delegate, so no manual `/export` is ever run. -Create the durable records for the plan's process tier, each as its own tracker comment, never combined: `full` = all four below; `light` = records 1–3 only, with `transcript_attachment: null` in the metrics JSON. The tier never changes CI/review coverage. Record 4 has a second, independent gate on top of tier — see §4. +Create the durable records for the plan's process tier, each as its own tracker comment, never combined: `full` = all four below; `light` = records 1–3 only, with `transcript_attachment: null` in the metrics JSON. The tier never changes CI/review coverage. Record 4 has a second, independent gate on top of tier — see §4. Every record here, and every state brief and handoff record this phase hands on, is written under `${CLAUDE_PLUGIN_ROOT}/skills/shared/references/context-economy.md`. ## Doc-accuracy self-check (before the retro) diff --git a/skills/spec/SKILL.md b/skills/spec/SKILL.md index c016cb3..a81f019 100644 --- a/skills/spec/SKILL.md +++ b/skills/spec/SKILL.md @@ -43,7 +43,7 @@ Draft Summary, Context/constraints, and Out of scope. Write the body as short na ### Existing Task -Read its body/comments directly and preserve settled decisions. Delegate only large parent-Epic or PR tails to `sy:sweep`. Edit the body only when research changes framing, and then as a deliberate full rewrite authored from scratch — never a merge around body content read back from the tracker, and it does not carry over rich text a description read cannot represent (see the selected adapter's `ADAPTER.md` on what that read does not guarantee). That governs research-phase edits; §7 Step 2 never writes the Task body at all, and posts its summary as a comment instead. Ensure the parent Epic is `in-progress` when active work begins; the Task stays in `backlog` until its plan is approved (then `ready`, per step 7). +Read its body/comments directly and preserve settled decisions. Delegate only large parent-Epic or PR tails to `sy:sweep`. Edit the body only when research changes framing, and then as a deliberate full rewrite authored from scratch — never a merge around body content read back from the tracker, and it does not carry over rich text a description read cannot represent (see the selected adapter's `ADAPTER.md` on what that read does not guarantee). That governs research-phase edits; §7 Step 2 never writes the Task body at all. Ensure the parent Epic is `in-progress` when active work begins; the Task stays in `backlog` until its plan is approved (then `ready`, per step 7). ## 3. Resolve standards and deep research @@ -108,7 +108,7 @@ Not every spec ends in a plan. When research shows the premise is already delive Nothing here starts until both mandatory §3 passes have run — the `sy:debate` pass over the core decision and the `sy:spec-gate` review of the drafted plan — with every spec-gate finding already dispositioned. -The plan itself has two clearly labeled parts, so a human reviewer and a fresh `/sy:ship` session each get only what they need without wading through the other's: +The plan itself has two clearly labeled parts, so a human reviewer and a fresh `/sy:ship` session each get only what they need without wading through the other's. Both parts are drafted under `${CLAUDE_PLUGIN_ROOT}/skills/shared/references/context-economy.md`; read it before writing either. **For your sign-off** (rationale and judgment calls): @@ -143,13 +143,13 @@ The ship profile never lowers review or build: `sy:gate` remains frontier tier a Present a short natural-prose summary: what you are going to do, why this way, the strongest alternative you rejected and why, the risks worth knowing, and what this deliberately excludes. A few paragraphs, read once and understood — no nested outline, no file inventory, no restatement of the `/sy:ship` section. What is being approved is the judgment; the mechanics exist for `/sy:ship`, and pasting them here buries the decision the user is being asked to make. -Send that summary as direct text first, in full — two acts, not one, and the summary is never folded into the question call or replaced by a pointer at it. Only then close the turn with a single `AskUserQuestion` call — approve as-is / request changes / other — per `${CLAUDE_PLUGIN_ROOT}/skills/shared/references/user-interaction.md`. Name the mutation the approval authorizes: on approval the run will post the full ACTIVE plan comment (and, when superseding, mark the prior plan SUPERSEDED), post this summary as a comment on the Task — Step 2 never writes the Task body (a body edit during research, §2, is governed separately by that section, not by this rule) — and set the Task `ready`. Under auto-mode this sign-off is the consent point for those writes, so it states them rather than implying them. This is the plan's sign-off gate: do not infer approval from a reply that doesn't answer it. +Send that summary as direct text first, in full — two acts, not one, and the summary is never folded into the question call or replaced by a pointer at it. Only then close the turn with a single `AskUserQuestion` call — approve as-is / request changes / other — per `${CLAUDE_PLUGIN_ROOT}/skills/shared/references/user-interaction.md`. Name the mutation the approval authorizes: on approval the run will post the full ACTIVE plan comment (and, when superseding, mark the prior plan SUPERSEDED) and set the Task `ready` — it does not touch the Task body (a body edit during research, §2, is governed separately by that section, not by this rule). Under auto-mode this sign-off is the consent point for those writes, so it states them rather than implying them. This is the plan's sign-off gate: do not infer approval from a reply that doesn't answer it. A `request changes` answer revises the draft and returns to this step; re-run `sy:spec-gate` only when that revision is material, per the re-dispatch rule in its reference. ### Step 2 — after approval, post the full plan -Both labeled parts are revealed here, in full, rather than at Step 1. Marking a superseded plan SUPERSEDED rather than leaving two ACTIVE is the retroactive-honesty invariant in `${CLAUDE_PLUGIN_ROOT}/skills/shared/references/write-integrity.md`: an overruled record is corrected on its own surface, never left standing. +Both labeled parts are revealed here, in full, rather than at Step 1. This step never writes the Task body — not for a pre-existing Task, and not even for one this run just created — so no existing body content is ever a target of this run's write. (A body edit during research, §2, is governed separately by that section, not by this rule.) Marking a superseded plan SUPERSEDED rather than leaving two ACTIVE is the retroactive-honesty invariant in `${CLAUDE_PLUGIN_ROOT}/skills/shared/references/write-integrity.md`: an overruled record is corrected on its own surface, never left standing. 1. if an older plan is ACTIVE, edit its comment to: @@ -168,8 +168,7 @@ Supersedes: v # omit for v1 ``` 3. verify by rereading plan headings/statuses that **exactly one** plan is ACTIVE. -4. post the Step-1 summary as a comment on the Task via the `tracker` skill (`post-comment`): `human` is the approach, the strongest rejected alternative, the risks, and the exclusions; `agent_detail` is the mutations the approval authorized. This step never writes the Task body — not for a pre-existing Task, and not even for one this run just created — so no existing body content is ever a target of this run's write. (A body edit during research, §2, is governed separately by that section, not by this rule.) -5. set the Task to `ready` via the `tracker` skill — the plan is approved and it is now shippable. +4. set the Task to `ready` via the `tracker` skill — the plan is approved and it is now shippable. The bar: a fresh session reading the Task and sole ACTIVE plan can implement and open the PR without missing design decisions. diff --git a/skills/tracker/CONTRACT.md b/skills/tracker/CONTRACT.md index d1e2cc9..5427a76 100644 --- a/skills/tracker/CONTRACT.md +++ b/skills/tracker/CONTRACT.md @@ -119,6 +119,8 @@ Generate usage from the on-disk transcript tree with the `usage_summarize` tool `shipyard.ship_metrics.v1` is **enforced**, not just documented: a body naming that id must carry exactly one fenced JSON block whose top-level `schema` key claims it, and `post-log`, `post-comment`, `create-issue` and `update-issue` alike refuse the whole write when that block does not match the schema — the identical gate on all four bodies, so a machine log written into an issue body is held to exactly what a comment is — and equally when the body carries more than one such block, since which is the log is then ambiguous and validating the first would write the rest unchecked. That count is not limited to well-formed fences: an unclosed block, a closing marker with trailing text, or a bare prose mention of the id all count as a candidate too, so naming the id anywhere outside the one valid block is also a refusal, not a silent pass-through. A malformed metrics log therefore cannot land and then be read as authoritative. Field definitions live in exactly one place, `${CLAUDE_PLUGIN_ROOT}/skills/ship/references/handoff-accounting.md`; the executable copy is `sy_tools/ship_metrics.py`. Every field is optional except `schema` and `task` — an unknown metric is posted as `null`, never as a plausible zero — with the few exceptions the model states and the reference explains. A body that neither names the id — literally or via a JSON `\uXXXX` escape — nor carries a block whose parsed content resolves to it passes through unvalidated. +**Body size is the second such guard, over the identical four writers.** A body longer than the selected adapter's limit is refused whole by `post-log`, `post-comment`, `create-issue` and `update-issue` alike, before the write is attempted, with a message naming the measured length, the limit, and the overflow — the tracker would refuse an oversized body outright anyway, though a refusal here is not proof it would have — and nothing here truncates a body to fit, because a silently shortened plan or retrospective reads as a complete one. The limit is **per adapter and best-effort**, not a spec: each adapter states its own number and where that number comes from, and a body under it is one the tracker has not been observed to refuse rather than one it promises to accept. Split oversized content across writes, or shorten it. + ### Attachments may degrade to a link `attach-artifact` uploads a file where the tracker supports it (Jira work-item attachments). Where it does not (GitHub issues have no CLI-scriptable attachment), the adapter substitutes an equivalent durable artifact (a private gist) and links it from a comment. Either way the artifact is secret-scanned before it leaves the machine, and the log comment references it by name/URL. This asymmetry is documented per adapter and in the deliberate-asymmetries section of the README. The lifecycle verbs act on whatever the adapter created, so `attachment-download` on a GitHub issue reads the gist back. diff --git a/skills/tracker/github/ADAPTER.md b/skills/tracker/github/ADAPTER.md index ceaa69c..a28ccd8 100644 --- a/skills/tracker/github/ADAPTER.md +++ b/skills/tracker/github/ADAPTER.md @@ -66,6 +66,8 @@ An issue's opaque id is its **URL** in everything this adapter returns. `gh` acc - **`link-pr`**: reference the issue from the PR body as a plain `#`, **not** a closing keyword — the done transition is owned by native project automation on merge, not by the PR text. - **`type-convert`** rewrites the board `Type` field on an existing issue, verified by the same bounded re-read every board write uses. +This adapter's body limit is **65,536 characters**, applied by the shared whole-write refusal in `../CONTRACT.md`. GitHub documents no such limit anywhere: the figure is attested by nothing but the API's own error string on a write that exceeds it, so treat it as observed behaviour that could move rather than a published bound. + ### `attach-artifact` and the attachment lifecycle — gist + link (deliberate asymmetry) GitHub issues have no CLI-scriptable file attachment, so the artifact is uploaded as a **secret** (private) gist and linked from a comment on the issue. Hand the rendered path to the `attach-artifact` tool: it checks the gate and runs both sanitisation passes — the same ones, in the same order, as on the Jira path — before creating the gist, and returns the gist URL as its evidence. The caller names no tracker; the asymmetry lives here, in the adapter. Privacy is verified by reading the created gist back rather than assumed from the flags passed: a public gist would publish a transcript irrevocably. diff --git a/skills/tracker/jira/ADAPTER.md b/skills/tracker/jira/ADAPTER.md index 687f40e..ddba072 100644 --- a/skills/tracker/jira/ADAPTER.md +++ b/skills/tracker/jira/ADAPTER.md @@ -60,6 +60,8 @@ Everything below is Jira-specific behaviour a caller can rely on. Where a verb i - **`link-pr`**: PRs surface in the Jira development panel when the branch or commit names the issue key. The verb's durable half is a comment whose `human` notes that a PR now exists and whose `agent_detail` is the PR URL, so the association survives regardless of dev-panel wiring. - **`type-convert`** rewrites the work item's type in place and verifies by reading it back. Some site workflows restrict type changes (required fields, hierarchy rules); it then fails loudly rather than leaving the type silently unchanged. Irreversible side effects — parent links, board membership — follow the type. +This adapter's body limit is **32,767 characters**, applied by the shared whole-write refusal in `../CONTRACT.md`. Atlassian's own JCMA migration KB states that on Cloud "it's not possible to bypass the 32,767 character limit for both description and comments" (citing JRACLOUD-59124); the `jira.text.field.character.limit` property behind it is documented and admin-tunable in Data Center only — JRACLOUD-63007 is Atlassian declining to expose it in Cloud without disputing the reporter's premise that the same default applies there, and JRACLOUD-68949 corroborates the description-field limit specifically. What is still left undocumented is the *unit* for an ADF body, which is why the limit stays best-effort. + Deleting a dependency link is not a contract verb: no workflow drives it, so it stays a manual `acli jira workitem link delete --id --yes` outside Shipyard. ## `attach-artifact` and the attachment lifecycle diff --git a/sy_tools/server.py b/sy_tools/server.py index 80603bb..e0901e5 100644 --- a/sy_tools/server.py +++ b/sy_tools/server.py @@ -131,7 +131,12 @@ async def create_issue( # back out of a tracker's own history. (title, body), scrub = _scrub_texts(title, body) _validate_machine_log(body) - created = await tracker.adapter().create_issue(issue_type=issue_type, title=title, body=body, parent=parent) + # Order is load-bearing at all four writers that run these two checks: the machine-log check must be + # able to refuse before `adapter()` is called at all, and the size limit is only readable off the + # adapter — so the lookup sits between them and is never hoisted above the first check. + adapter = tracker.adapter() + _validate_body_size(body, adapter.body_limit) + created = await adapter.create_issue(issue_type=issue_type, title=title, body=body, parent=parent) return {**created, "scrub": scrub} @@ -165,7 +170,9 @@ async def update_issue( _required(issue=issue) (body,), scrub = _scrub_texts(body) _validate_machine_log(body) - updated = await tracker.adapter().update_issue(issue, body) + adapter = tracker.adapter() + _validate_body_size(body, adapter.body_limit) + updated = await adapter.update_issue(issue, body) return {**updated, "scrub": scrub} @@ -339,7 +346,9 @@ async def post_comment( # Kept as a defensive backstop, not as routing: `post-log` assembles and validates its own body, and # what this catches here is a claim that is prose-only, malformed, or ambiguous — never a valid log. _validate_machine_log(body) - posted = await tracker.adapter().post_comment(issue, body) + adapter = tracker.adapter() + _validate_body_size(body, adapter.body_limit) + posted = await adapter.post_comment(issue, body) return {**posted, "scrub": scrub} @@ -377,7 +386,9 @@ async def post_log( (title, payload_json), scrub = _scrub_texts(title, payload_json) body = f"# {title.strip()}\n\n```json\n{payload_json}\n```\n" _validate_machine_log(body) - posted = await tracker.adapter().post_comment(issue, body) + adapter = tracker.adapter() + _validate_body_size(body, adapter.body_limit) + posted = await adapter.post_comment(issue, body) return {**posted, "scrub": scrub} @@ -503,6 +514,16 @@ def _claims_within(parsed: object) -> bool: return False +def _validate_body_size(body: str, limit: int) -> None: + if len(body) > limit: + raise ToolError( + f"this body is {len(body)} characters and the limit is {limit}, so it was refused: " + f"{len(body) - limit} over. The write was not attempted, because a tracker that refuses an " + "oversized body refuses it whole — nothing partial lands and nothing is truncated for you. " + "Split the content across writes, or shorten it and send the shorter body." + ) + + def _validate_machine_log(body: str) -> None: """Reject a malformed `shipyard.ship_metrics.v1` block before the body it sits in is written. diff --git a/sy_tools/tests/test_server.py b/sy_tools/tests/test_server.py index a27e37d..cdb4672 100644 --- a/sy_tools/tests/test_server.py +++ b/sy_tools/tests/test_server.py @@ -98,6 +98,9 @@ class _Recorder: """ name = "recorder" + # A real attribute, not left to `__getattr__`: that returns an async callable for any name, so every + # 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: self.calls: list[tuple[str, tuple, dict]] = [] @@ -392,6 +395,65 @@ async def test_post_log_refuses_a_title_that_spans_lines_before_the_adapter_is_t assert result.is_error is True, f"a multi-line title was posted: {result.content}" +STUB_BODY_LIMIT = 400 +"""A small stand-in for the real per-adapter limit, so a case can be built out of a short string. + +Building a genuine 32,767-character body would pin this suite to one adapter's number and make every +case here re-derive it; the guard's behaviour is the same at any limit. +""" + +SIZE_WRITES = [ + ("create-issue", + lambda filler: {"issue_type": "task", "title": "T", "body": filler}, + lambda filler: filler), + ("update-issue", + lambda filler: {"issue": "PROJ-1", "body": filler}, + lambda filler: filler), + ("post-comment", + lambda filler: {"issue": "PROJ-1", "human": "TL;DR: sized.", "agent_detail": filler}, + lambda filler: "TL;DR: sized." + server._AGENT_DETAIL_OPEN + filler + server._AGENT_DETAIL_CLOSE), + ("post-log", + lambda filler: {"issue": "PROJ-1", "title": "Claude Code usage", "payload": {"note": filler}}, + lambda filler: f'# Claude Code usage\n\n```json\n{json.dumps({"note": filler}, indent=2)}\n```\n'), +] +"""Each writer the size guard covers, its arguments for a filler string, and the body it assembles. + +The second lambda is what makes `post-comment` and `post-log` testable at a boundary at all: neither +sends the string it was given, so a case sized against the argument would be sizing the wrong text. +""" + + +@pytest.mark.anyio +@pytest.mark.parametrize(("tool", "arguments", "assembled"), SIZE_WRITES, ids=[t for t, _a, _b in SIZE_WRITES]) +@pytest.mark.parametrize("overflow", [1, 0], ids=["one over the limit", "exactly at the limit"]) +async def test_a_body_over_the_adapters_limit_is_refused_whole_and_one_at_it_still_writes( + monkeypatch, tool, arguments, assembled, overflow +): + """Both boundaries, on every writer, because either half alone is a guard that looks like it works. + + Over the limit the tracker refuses the write outright, so the useful answer is a refusal here that + names the measured length — a caller with an oversized body has to know how much to cut, and a + length it can only guess at is what makes the retry a second failed write. At the limit is the half + that catches an off-by-one: a guard one character early silently costs every writer a character of + every body, and nothing else in this suite would notice. + """ + recorder = _Recorder() + recorder.body_limit = STUB_BODY_LIMIT + monkeypatch.setattr(server.tracker, "adapter", lambda: recorder) + filler = "x" * (STUB_BODY_LIMIT + overflow - len(assembled(""))) + body = assembled(filler) + assert len(body) == STUB_BODY_LIMIT + overflow, f"the case built {len(body)} characters, not the length it tests" + async with mcp.Client(server.mcp) as client: + result = await client.call_tool(tool, arguments(filler)) + if overflow: + assert result.is_error is True, f"{tool} wrote {len(body)} characters past a {STUB_BODY_LIMIT} limit" + assert str(len(body)) in _text(result), f"the refusal must name the measured length: {_text(result)}" + assert not recorder.calls, f"{tool} reached the adapter with an oversized body: {recorder.calls}" + else: + assert result.is_error is False, result.content + assert recorder.calls, f"{tool} refused a body that was exactly at the limit" + + @pytest.mark.anyio async def test_a_tracker_failure_comes_back_as_a_tool_result(monkeypatch): """No per-tool try/except: the SDK already turns a raising tool into an `isError` result. diff --git a/sy_tools/tracker/__init__.py b/sy_tools/tracker/__init__.py index 5ab49f3..f66b45c 100644 --- a/sy_tools/tracker/__init__.py +++ b/sy_tools/tracker/__init__.py @@ -117,6 +117,23 @@ async def add_label(self, issue: str, label: str) -> dict: """Add `label` to `issue`, preserving the labels already on it, and return the resulting set.""" ... + body_limit: int + """The largest body this tracker is believed to take, in characters of the Markdown body Shipyard + sends. Best-effort, not a guarantee. + + Annotation only, with no value: a default here would put a non-verb into `vars()`, which is where + `sy_tools/tests/tracker/test_canonical.py` pins the canonical verb set. + + It guards issue descriptions as well as comments, which is why it is not `comment_body_limit`. + No figure here is firm. Each adapter declares its own, and records that figure's provenance — + what attests it and how far that attestation reaches — in its own `ADAPTER.md`, since provenance + is inherently tracker-specific and this Protocol names no tracker. + + So a body under the limit is one this tracker has not been observed to refuse, not one it promises + to accept. The point of the number is to turn the common overflow into a refusal a caller can act + on before the write, not to certify the boundary. + """ + async def post_comment(self, issue: str, body: str) -> dict: """Post `body` as a Markdown comment on `issue` and return the comment the write created.""" ... diff --git a/sy_tools/tracker/github/adapter.py b/sy_tools/tracker/github/adapter.py index acf56f7..4211dea 100644 --- a/sy_tools/tracker/github/adapter.py +++ b/sy_tools/tracker/github/adapter.py @@ -89,6 +89,9 @@ class GithubAdapter: """ name = "github" + # Undocumented: GitHub publishes no body limit anywhere, and this figure is attested by nothing but + # the API's own error string on a write that goes over it. + body_limit: int = 65_536 def __init__(self) -> None: self._boards: dict[str, dict[str, Any]] = {} diff --git a/sy_tools/tracker/jira/adapter.py b/sy_tools/tracker/jira/adapter.py index 7945a9b..cf10473 100644 --- a/sy_tools/tracker/jira/adapter.py +++ b/sy_tools/tracker/jira/adapter.py @@ -97,6 +97,11 @@ class JiraAdapter: """The canonical tracker verbs, implemented against the Jira Cloud REST API.""" name = "jira" + # Characters. Atlassian's JCMA migration KB states the 32767 limit applies to both description and + # comments on Cloud (citing JRACLOUD-59124); the jira.text.field.character.limit property behind it + # is tunable in Data Center only (JRACLOUD-63007); JRACLOUD-68949 corroborates the + # description-field limit. Its unit is still undocumented under ADF. + body_limit: int = 32_767 def __init__(self) -> None: self._account_id: str | None = None