diff --git a/README.md b/README.md index 92ab85e..0a9c8e1 100644 --- a/README.md +++ b/README.md @@ -16,15 +16,18 @@ harness — not just one vendor's tool. ## Status -Pre-release, v0.1.0. 11 of the 14 planned tools are fully implemented and -dogfooded (KnoTrack tracks its own build using itself — see +Pre-release, v0.1.0. 12 of the 14 planned tools are fully implemented; 11 of +those are dogfooded (KnoTrack tracks its own build using itself — see [`scripts/seed-self.ts`](scripts/seed-self.ts) and `docs/ROADMAP.md`'s T1). -The remaining 3 ship as stub slices (2026-09-18, T2.11/T2.13/T2.14) — not -yet usable end to end: `kt_check_drift` returns a well-formed empty scan -(real drift heuristics land in T6), and the two sync tools validate their -inputs and the adapter precondition, returning a clear `CONFLICT` -("adapter not configured") or — once an adapter is configured — a "sync is -not available in this build" error; full GitHub/Linear sync ships in T5. +`kt_sync_to_github` is implemented as of T5.2 (2026-09-19) — idempotent +one-way sync of a track to a GitHub Issue — and **offline-verified**, but not +yet dogfooded: a real-repo dogfood run is its one remaining gate. The +remaining 2 ship as stub +slices: `kt_check_drift` returns a well-formed empty scan (real drift +heuristics land in T6), and `kt_sync_to_linear` validates its inputs and +the adapter precondition, returning a clear `CONFLICT` ("adapter not +configured") or — once an adapter is configured — a "sync is not available +in this build" error; Linear sync ships in T5.3. Every change lands through a mandatory adversarial-review gate before it's considered reviewed: deterministic checks (build, lint, typecheck, unit, @@ -40,22 +43,22 @@ include full diffs and raw model output. ## Tools -| Tool | Status | Purpose | -| --------------------------- | ----------- | --------------------------------------------------------------------------------------- | -| `kt_register_project` | implemented | Register (or upsert) a project by its source (`github`, `local`, etc.) | -| `kt_get_project_status` | implemented | Current status summary: tracks, items, drift flags | -| `kt_create_track` | implemented | Create a track (a sequenced line of work) under a project | -| `kt_create_item` | implemented | Create an item within a track, auto- or explicitly-sequenced | -| `kt_record_session_summary` | implemented | Record a session's summary and re-check for drift | -| `kt_list_tracks` | implemented | List a project's tracks, optionally filtered by status | -| `kt_get_track` | implemented | Track detail: items plus dependency graph | -| `kt_get_next_steps` | implemented | Suggested next items given current status and dependencies | -| `kt_record_decision` | implemented | Record a decision and the context behind it | -| `kt_update_item_status` | implemented | Move an item's status forward (or flag it blocked) | -| `kt_check_drift` | stub | On-demand drift scan (T2.11 stub: empty scan until heuristics ship in T6) | -| `kt_render_roadmap` | implemented | Render a roadmap view from tracked items | -| `kt_sync_to_github` | stub | One-way sync to GitHub Issues (T2.13 stub: adapter precondition only; sync ships in T5) | -| `kt_sync_to_linear` | stub | One-way sync to Linear (T2.14 stub: adapter precondition only; sync ships in T5) | +| Tool | Status | Purpose | +| --------------------------- | ----------- | ------------------------------------------------------------------------------------------- | +| `kt_register_project` | implemented | Register (or upsert) a project by its source (`github`, `local`, etc.) | +| `kt_get_project_status` | implemented | Current status summary: tracks, items, drift flags | +| `kt_create_track` | implemented | Create a track (a sequenced line of work) under a project | +| `kt_create_item` | implemented | Create an item within a track, auto- or explicitly-sequenced | +| `kt_record_session_summary` | implemented | Record a session's summary and re-check for drift | +| `kt_list_tracks` | implemented | List a project's tracks, optionally filtered by status | +| `kt_get_track` | implemented | Track detail: items plus dependency graph | +| `kt_get_next_steps` | implemented | Suggested next items given current status and dependencies | +| `kt_record_decision` | implemented | Record a decision and the context behind it | +| `kt_update_item_status` | implemented | Move an item's status forward (or flag it blocked) | +| `kt_check_drift` | stub | On-demand drift scan (T2.11 stub: empty scan until heuristics ship in T6) | +| `kt_render_roadmap` | implemented | Render a roadmap view from tracked items | +| `kt_sync_to_github` | implemented | One-way idempotent sync of a track to a GitHub Issue (T5.2; real-repo dogfood gate pending) | +| `kt_sync_to_linear` | stub | One-way sync to Linear (T2.14 stub: adapter precondition only; sync ships in T5.3) | Full request/response contracts for every tool, implemented or planned, are in [`docs/TRD.md`](docs/TRD.md). diff --git a/docs/PRD.md b/docs/PRD.md index 5aab223..067b37b 100644 --- a/docs/PRD.md +++ b/docs/PRD.md @@ -396,7 +396,7 @@ There is no `advisory_notice` field echoed on every response and no `blocking_su | `SEQUENCE_SKIP` | `info` | An item at `sequence_position = k` is `done` while another item in the *same track* at a lower `sequence_position` is `pending` or `blocked` — work finished out of its declared order. Informational, not necessarily wrong. | | `UNDOCUMENTED_DECISION` | `warning` | A Decision exists for a track, and no Event for that same track has a `created_at` later than the Decision's — a pivot was logged but nothing since shows it was acted on (§4.9). | | `ORPHAN_ITEM` | `warning` | An item's `depends_on_item_id` points to an item in a *different* track. Defensive only — `kt_create_item` (§4.7) already restricts this at write time. | -| `SYNC_DRIFT` | `warning` | The project has credentials configured for an adapter, and a track's most recent activity is later than that adapter's last successful sync of it. **Schema exists (`tracks.last_github_sync_at`/`last_linear_sync_at`); the rule itself is not yet built** — the sync adapters (§4.13, §4.14) and this scan aren't implemented yet either. | +| `SYNC_DRIFT` | `warning` | The project has credentials configured for an adapter, and a track's most recent activity is later than that adapter's last successful sync of it. **Schema exists (`tracks.last_github_sync_at`/`last_linear_sync_at`); the `SYNC_DRIFT` rule itself and the `kt_check_drift` scan are not yet built (T6).** `kt_sync_to_github` (§4.13) is implemented (T5.2) and writes `last_github_sync_at` on each successful sync; the Linear sync (§4.14) is still a stub (T5.3). | **Business rules / edge cases:** - Exceeding the scan's track/item caps or its wall-clock time budget (all operator-configured via environment variables — `docs/TRD.md` §6.3/§7) is **not** an error: the call returns whatever was scanned so far with `truncated: true`, `scanned_track_count`, and `total_track_count`, so the caller can see exactly how partial the result is. @@ -427,22 +427,24 @@ There is no `advisory_notice` field echoed on every response and no `blocking_su ### 4.13 `kt_sync_to_github` -**Description:** **Registered with its real, TRD-accurate input schema. In this build it ships the T2.13 stub slice** — it validates the project and track, then requires a configured GitHub adapter; with none stored it returns `CONFLICT` (see below) and makes no HTTP call. The actual GitHub sync is T5.2. This section documents its target contract: one GitHub sync operation, scoped to a single track, per call — there is no `pull`/`push`/direction choice and no item-level sync target in v1. +**Description:** **Implemented (T5.2, 2026-09-19 — replaces the earlier T2.13 stub slice).** One GitHub sync operation, scoped to a single track, per call — there is no `pull`/`push`/direction choice and no item-level sync target in v1. It creates a GitHub Issue for the track on first sync and updates it on subsequent syncs, idempotently: the authoritative "does an issue already exist for this track" record is a `track_external_links` row (`migrations/009`), never a GitHub search. Idempotency is hardened with a durable creation-intent — a `pending` link row is committed *before* the outbound create, so a crash between GitHub accepting the create and the server recording the result never silently produces a duplicate (the next call recovers via a hidden `` body marker). The created/updated issue's `html_url` is recorded on the track and surfaced on `kt_get_track` as `track.github_issue_url` (that is what "records the issue URL on the track" means — there is no separate URL column on `tracks`). Retries/backoff are intentionally out of scope for v1: a transient upstream failure surfaces as `{ ok: false }` for the caller to retry. **Inputs:** `project_id` (required), `track_id` (required). There is no `direction`, `github_repo` override, or `item_id` input. **Output (by outcome):** -- Success: `{ ok: true }`. +- Success: `{ ok: true }` (whether the issue was created, updated, or was already up to date — a no-op re-sync makes no HTTP call). - Operational failure (bad token, repo/issue not found, rate-limited, network timeout) — **a successful tool call, not an MCP-level error**: `{ ok: false, error: ": " }`. Defined prefixes: `GITHUB_AUTH_FAILED`, `GITHUB_NOT_FOUND`, `GITHUB_RATE_LIMITED`, `GITHUB_TIMEOUT` (exceeds `KNOTRACK_GITHUB_SYNC_TIMEOUT_MS`, default 8000ms), `GITHUB_UNKNOWN_ERROR`. **Business rules / edge cases:** -- **Two distinct failure surfaces, deliberately kept separate.** A precondition the caller can fix by calling a different tool first — no GitHub credentials stored for this project (no row in `adapters` for `(project_id, "github")`) — is a real tool-level error, `CONFLICT`, via the standard error envelope. Everything about actually talking to GitHub itself is **not** an MCP-level error at all — the call succeeds and returns the discriminated `{ok: false, error: ...}` result instead, because these are expected, retryable operational outcomes, not contract violations. +- **Two distinct failure surfaces, deliberately kept separate.** A precondition the caller can fix by configuring the adapter first — no GitHub credentials stored for this project (no row in `adapters` for `(project_id, "github")`), **or** a stored GitHub adapter with no `repo` configured (so no API URL can be formed) — is a real tool-level error, `CONFLICT`, via the standard error envelope, and makes no HTTP call. Everything about actually talking to GitHub itself is **not** an MCP-level error at all — the call succeeds and returns the discriminated `{ok: false, error: ...}` result instead, because these are expected, retryable operational outcomes, not contract violations. +- **Idempotency / no duplicates.** The target repo is read from the stored adapter config, never a tool argument. Repeated syncs of the same track never create a second Issue: create-vs-update is decided from the `track_external_links` row, a re-sync with no content change is a no-op (no HTTP call), and the link is bound to the exact `owner/repo` it was created against — a later silent change to the adapter's repo makes the sync refuse rather than update an unrelated issue. A stale-link `404` on update returns `GITHUB_NOT_FOUND` and does **not** auto-recreate. - *An earlier draft of this document described a `direction`-based pull/push design with dedup and partial-result reporting across a batch of Issues — the shipped, mandated 14-tool contract has no such design. Each call is one scoped operation on one track, not a bulk import; there is no bidirectional merge or conflict-resolution logic to document.* **Acceptance criteria:** -- **Given** a project without GitHub credentials configured, **when** `kt_sync_to_github` is called, **then** the call fails with `CONFLICT`. -- **Given** GitHub returns a 403 rate-limit response (once this tool is implemented), **when** called, **then** the response is `{ ok: false, error: "GITHUB_RATE_LIMITED: ..." }` — a successful tool call, not a thrown error. -- **Given** this build, **when** `kt_sync_to_github` is called for an existing project and track with no GitHub adapter configured, **then** the missing-adapter `CONFLICT` (above) is the shipped behavior and no HTTP call is made; the operational GitHub API path (the `{ ok: false, error }` outcomes) is not yet implemented (T5.2). An adapter configured via `kt_register_project` (which provisions adapters from inline credentials) yields an `INTERNAL_ERROR` (`github sync is not available in this build`), never a false success. +- **Given** a project without a GitHub adapter configured (or a GitHub adapter with no `repo`), **when** `kt_sync_to_github` is called, **then** the call fails with `CONFLICT` and no HTTP call is made. +- **Given** a configured GitHub adapter, **when** `kt_sync_to_github` is called for a track with no existing link, **then** an Issue is created, the link is recorded, and `kt_get_track`'s `track.github_issue_url` returns the Issue URL; a second call with unchanged content is a no-op (no HTTP call) and a call after the track changes updates the same Issue. +- **Given** GitHub returns a 403 rate-limit response, **when** `kt_sync_to_github` is called, **then** the response is `{ ok: false, error: "GITHUB_RATE_LIMITED: ..." }` — a successful tool call, not a thrown error. +- **Given** a `pending` link left by a crashed prior create, **when** `kt_sync_to_github` is called again, **then** it recovers via the hidden marker (adopting the existing Issue if found) rather than creating a duplicate. *Verified offline (fake `GitHubClient`, no network in CI); the real-repo run against `SathiaAI/KnoTrack` with a fine-grained PAT is the one remaining acceptance gate (docs/ROADMAP.md T5.2).* ### 4.14 `kt_sync_to_linear` @@ -522,7 +524,7 @@ Because KnoTrack collects no central telemetry (§5.6), every metric below is so 2. **Drift trend, not drift count.** The ratio of `SEQUENCE_SKIP` flags (§4.11) that get resolved, or simply stop recurring as work catches up to its declared order, versus flags that persist open across repeated checks. A tool that's working sees this trend toward resolved, not toward a silently accumulating pile of open flags nobody looks at. 3. **`kt_get_next_steps` actually consulted before work starts.** Correlating ordinary HTTP/application access logs for `kt_get_next_steps` calls against the timestamps of subsequent Item status changes and Events — if items are consistently touched shortly after a next-steps call recommended them, the advisory loop is being used as intended. Note this is derived from access logs, not from the Event table, because `kt_get_next_steps` is deliberately side-effect-free (§4.5) and creates no Event of its own. 4. **Rendered-roadmap freshness, if the caller chooses to persist it.** `kt_render_roadmap` (§4.12) has no notion of a target file — it returns rendered content as a string — so freshness is only measurable if and however the calling agent chooses to persist that string (e.g. committing it into the repo as `ROADMAP.md`). Where an installer adopts that convention, the time elapsed between a DB state change and the next commit of the rendered content is still a meaningful trust signal for the snapshot; where they don't, this metric simply doesn't apply to that installation. -5. **Adapter usage where configured.** For a project with GitHub or Linear credentials configured (a row exists in `adapters` for that project/type — §5.3), the frequency of successful sync calls relative to manual, un-synced Item creation — if a project has an adapter configured but the sync tools are never (or, in this build, cannot yet be — §4.13/§4.14) exercised, that's a signal the adapter isn't earning its complexity for that installer. +5. **Adapter usage where configured.** For a project with GitHub or Linear credentials configured (a row exists in `adapters` for that project/type — §5.3), the frequency of successful sync calls relative to manual, un-synced Item creation — if a project has an adapter configured but the sync tools are never exercised (for a Linear adapter, note §4.14 is still a stub in this build), that's a signal the adapter isn't earning its complexity for that installer. 6. **Time-to-first-registered-project per deploy path (maintainer-run, manual QA).** Not automated telemetry — a manual checklist the maintainers run themselves against each of the three deploy targets in §5.5, timing from `git clone` to a successful first `kt_register_project` call. Target: under 30 minutes on each path, re-verified whenever a deploy guide or dependency changes. As of 2026-09-07 this has only actually been run for Railway (well under target — see `docs/ROADMAP.md` T3/T4); Render+Supabase and Fly.io have a draft runbook (`docs/deploy/client-verification-runbook.md`) but it hasn't actually been run end-to-end yet, so this metric doesn't apply to them until `T7.1`/`T7.2` close. --- @@ -532,7 +534,7 @@ Because KnoTrack collects no central telemetry (§5.6), every metric below is so - **Work dispatch or orchestration of any kind.** No tool in this document assigns, triggers, queues, or executes work against any agent, CI system, or external runner. `kt_get_next_steps` is advisory only (§4.5); this boundary is treated as permanent product identity, not a temporary v1 gap — see §2.2. - **Team/multi-user auth beyond a single shared bearer-token pool.** There is no role-based access control, no SSO/OAuth login flow, no per-device or per-user token identity, and no concept of a restricted or read-only token in v1 — every valid token accepted by an instance (§5.3) has full read/write access to every project it hosts, and the server has no way to tell which configured token a given call used. A small team (persona in §3.2) shares one instance and, in practice, one token per developer drawn from the same pool, all with equal privileges. Formal per-user roles and scoped/read-only tokens are a natural v2 addition once real multi-user usage shows which restrictions are actually wanted. - **Real-time push UI.** `kt_get_project_status` and all other read tools are pull/poll-based only. There is no WebSocket/SSE channel, no live-updating dashboard, and no in-app or external notification (Slack, email, etc.) fired when drift is detected — a human or agent must proactively call a status/drift tool to find out. A future web UI, if built, would poll these same MCP tools rather than requiring a new push mechanism. -- **Automatic conflict resolution for adapter sync, and bidirectional/bulk sync generally.** `kt_sync_to_github` and `kt_sync_to_linear` (§4.13, §4.14 — not yet implemented in this build) are each a single, scoped, one-way push per call, with no bulk import, no direction choice, and no merge logic for the case where the same piece of work has diverged independently on both sides. +- **Automatic conflict resolution for adapter sync, and bidirectional/bulk sync generally.** `kt_sync_to_github` (§4.13) and `kt_sync_to_linear` (§4.14 — the Linear adapter is still a stub in this build) are each a single, scoped, one-way push per call, with no bulk import, no direction choice, and no merge logic for the case where the same piece of work has diverged independently on both sides. - **Automatic parsing of arbitrary local roadmap/spec file formats, and importing existing GitHub/Linear issues.** As stated in §1.3, KnoTrack does not itself ingest free-text planning documents from a local folder; the calling agent reads them and populates Tracks/Items via `kt_create_track`/`kt_create_item`. The GitHub and Linear adapters are one-way pushes of existing KnoTrack state outward, not an import path — there is no tool in v1 that pulls GitHub Issues or Linear Issues into KnoTrack Items. - **Centralized, maintainer-run hosting or analytics.** There is no multi-tenant SaaS version of KnoTrack, and no telemetry collection of any kind (§5.6). "Open source, self-hosted" is the whole distribution model for v1, not a stepping stone the PRD assumes will change. - **A standalone chat or web-app interface.** KnoTrack in v1 is purely an MCP tool surface. Whatever conversational interface a user experiences is provided entirely by the calling agent harness (Claude Code, Windsurf, etc.), not by KnoTrack itself. @@ -548,7 +550,7 @@ Because KnoTrack collects no central telemetry (§5.6), every metric below is so - **Event** — An append-only log entry created by `kt_record_session_summary`, recording what happened in a session: a `summary_text`, the files touched, and any items touched. Events carry no per-caller identity (§5.3) and no self-reported drift opinion — drift is structural only. Events are never edited or deleted once written; recording one also re-runs the drift-detector's rules scoped to that Event's track. - **Decision** — An explicit, append-only record of an intentional pivot or plan change, scoped to one Track: a `title`, a `rationale` (why), a `what_changed` description (what concretely changed), and an `effect` (`note` | `open_pivot` | `resolve_pivot`, T2.16 — default `note`, which has no lifecycle side effect at all). Decisions are never inferred from a status change or a boolean flag — they only exist because `kt_record_decision` was deliberately called with real content. Only `open_pivot`/`resolve_pivot` move a Track into or out of `pivot_pending` (§4.9); a Decision never directly suppresses any drift flag, though it changes the input to the `UNDOCUMENTED_DECISION` flag specifically. - **Drift** — A structurally-computed mismatch between the declared plan and what actually happened, never from a self-reported opinion. Evaluated against a six-type flag catalog (`STALE_TRACK`, `DEPENDENCY_GAP`, `SEQUENCE_SKIP`, `UNDOCUMENTED_DECISION`, `ORPHAN_ITEM`, `SYNC_DRIFT` — full definitions in §4.11); as of this build, only `SEQUENCE_SKIP` is actually raised anywhere, via `kt_record_session_summary`'s scoped per-track re-check, since the full-project `kt_check_drift` scan is not yet implemented. -- **Adapter** — A per-project integration to an external source-of-truth system (GitHub or Linear) that provides one-way, per-track push sync, gated on the server holding the relevant credential for that project (a row in the `adapters` table) — not a separate "enable this adapter" flag. Adapter credentials are always server-side only, never passed through any MCP tool's output. Both sync tools ship their precondition stub in this build (project/track validation plus a `CONFLICT` when no adapter is configured); the outward push itself is not yet implemented (§4.13, §4.14 — T5.2/T5.3). +- **Adapter** — A per-project integration to an external source-of-truth system (GitHub or Linear) that provides one-way, per-track push sync, gated on the server holding the relevant credential for that project (a row in the `adapters` table) — not a separate "enable this adapter" flag. Adapter credentials are always server-side only, never passed through any MCP tool's output. `kt_sync_to_github` (§4.13) is implemented (T5.2 — a real one-way, per-track push that creates/updates the linked Issue idempotently); `kt_sync_to_linear` (§4.14) still ships only its precondition stub (project/track validation plus a `CONFLICT` when no adapter is configured), with its outward push deferred to T5.3. - **Advisory** — The general operating principle behind `kt_get_next_steps` and, in effect, every other KnoTrack tool: the system recommends, tracks, and reports, but never assigns, dispatches, or blocks a human/agent's actual actions. --- @@ -565,7 +567,7 @@ Track- and item-level dependencies are stored as **join tables** (`track_depende **Adapter** `id (uuid pk)`, `project_id (uuid fk)`, `type (text — "github" | "linear")`, `encrypted_credential (bytea — a packed iv(12 bytes) || authTag(16 bytes) || ciphertext blob, §5.3)`, `config (jsonb — non-secret metadata only, e.g. {"owner": "acme", "repo": "widgets"})`, `key_version (integer, default 1 — which generation of `KNOTRACK_ENCRYPTION_KEY` encrypted this row; bumped by the key-rotation process, §5.3)`, `created_at`. Unique on `(project_id, type)` — at most one adapter row per project per type. -**Track** — **there is no `status` column (T2.16).** `id (uuid pk)`, `project_id (uuid fk)`, `title (text)`, `pivot_decision_id (uuid, nullable, composite fk -> decisions(id, track_id, effect) — NULL means no active pivot, §4.4/§4.9)`, `pivot_effect (text, GENERATED ALWAYS AS ('open_pivot') STORED — migration 008)` — exists only so `pivot_decision_id`'s FK can be a 3-column composite that also pins the referenced decision's `effect` to `open_pivot`, not just its identity/track, `source_doc_ref (text, nullable)`, `last_github_sync_at (timestamptz, nullable)`, `last_linear_sync_at (timestamptz, nullable — both added for the future SYNC_DRIFT rule, §4.11; not yet written by anything, since the sync tools aren't implemented)`, `created_at`, `updated_at`. `status` (`on_track|pivot_pending|blocked|done`) is computed at read time by the `track_readiness` database view from `pivot_decision_id`, the track's own item completion, and its direct dependencies' state — no column to write, so no tool writes it directly. Track-level dependencies: a `track_dependencies (track_id, depends_on_track_id, project_id, created_at)` join table, no self-dependency allowed; `project_id` (migration 008, backfilled from `tracks.project_id`, NOT NULL) is enforced by two composite FKs to `tracks(id, project_id)` so both sides of an edge are schema-guaranteed to belong to the same project — closing a gap that was previously only enforced at the application layer (`kt_create_track`). There is no `description` column. Also unique on `(id, project_id)` (migration 008, backing those composite FKs). +**Track** — **there is no `status` column (T2.16).** `id (uuid pk)`, `project_id (uuid fk)`, `title (text)`, `pivot_decision_id (uuid, nullable, composite fk -> decisions(id, track_id, effect) — NULL means no active pivot, §4.4/§4.9)`, `pivot_effect (text, GENERATED ALWAYS AS ('open_pivot') STORED — migration 008)` — exists only so `pivot_decision_id`'s FK can be a 3-column composite that also pins the referenced decision's `effect` to `open_pivot`, not just its identity/track, `source_doc_ref (text, nullable)`, `last_github_sync_at (timestamptz, nullable)`, `last_linear_sync_at (timestamptz, nullable — both added for the SYNC_DRIFT rule, §4.11; `last_github_sync_at` is now written by `kt_sync_to_github` on each successful sync (T5.2), stamped to the payload snapshot time; `last_linear_sync_at` is not yet written, pending the Linear sync (T5.3))`, `created_at`, `updated_at`. `status` (`on_track|pivot_pending|blocked|done`) is computed at read time by the `track_readiness` database view from `pivot_decision_id`, the track's own item completion, and its direct dependencies' state — no column to write, so no tool writes it directly. Track-level dependencies: a `track_dependencies (track_id, depends_on_track_id, project_id, created_at)` join table, no self-dependency allowed; `project_id` (migration 008, backfilled from `tracks.project_id`, NOT NULL) is enforced by two composite FKs to `tracks(id, project_id)` so both sides of an edge are schema-guaranteed to belong to the same project — closing a gap that was previously only enforced at the application layer (`kt_create_track`). There is no `description` column. Also unique on `(id, project_id)` (migration 008, backing those composite FKs). **Item** `id (uuid pk)`, `track_id (uuid fk)`, `title (text)`, `sequence_position (integer, not unique per track_id — see §4.7's shift-on-insert behavior)`, `status (text, CHECK-constrained: pending|in_progress|done|blocked, default pending — written only by kt_update_item_status, §4.10)`, `created_at`, `updated_at`. Item-level dependencies: an `item_dependencies (item_id, depends_on_item_id, created_at)` join table restricted at the application layer (§4.7) to pairs within the same track. There is no `description`, `file_patterns`, or `external_ref` column. diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index 3f93fc5..59085dc 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -949,22 +949,78 @@ this Track. ## T5 — GitHub + Linear adapters -**Status:** `blocked`. +**Status:** `on_track` (corrected 2026-09-19 — the two blockers named +below are both cleared: `T4` went `done` 2026-09-07 and `T2.16` shipped +2026-09-08, so the stale `blocked` no longer holds). `T5.1` is **done + +verified 2026-09-19** and `T5.2` is **implemented + offline-verified +2026-09-19** (real-repo dogfood gate pending — see those items); `T5.3` / +`T5.4` remain open. **depends_on:** `T4`, `T2.16` (added 2026-08-29 — `SYNC_DRIFT`'s notion of "the track's most recent change" and any future logic that reads track completion to decide what to push should be built against the corrected track-status model, not the currently-broken one). -1. **T5.1 — Credential encryption at rest implemented.** Acceptance: +1. **T5.1 — Credential encryption at rest implemented. ✅ Done + verified + 2026-09-19.** Acceptance: `adapters.encrypted_credential` is written using envelope encryption (e.g. AES-256-GCM under a server-held master key), the plaintext token is never persisted, and a unit test confirms the stored bytes are ciphertext with a correct decrypt round-trip. depends_on: `T2.15`. -2. **T5.2 — `kt_sync_to_github` fully implemented.** Acceptance: given a - stored encrypted GitHub credential, calling `kt_sync_to_github` - creates/updates a linked GitHub Issue for a KnoTrack item and records - the issue URL on the item, verified against one real test repository. - depends_on: `T2.12`, `T5.1`. + **Verification (2026-09-19):** implementation lives in + `src/crypto/credential-cipher.ts` — AES-256-GCM, a fresh random 12-byte + IV per call, packed `iv||authTag(16)||ciphertext` into the single + `adapters.encrypted_credential` column; the 256-bit master key is held + server-side (`config.encryptionKey`) and never persisted with the + record. Acceptance is met by two tests: `tests/unit/credential-cipher.test.ts` + (round-trip, unique IV per call, stored bytes never contain the + plaintext, wrong-key throws, tampered-ciphertext throws) **and** + `tests/integration/register-project.test.ts` (reads + `encrypted_credential` back from the `adapters` table and asserts + `decryptCredential(stored, key)` returns the original token — the + "stored value is ciphertext with a correct decrypt round-trip" proof at + the DB layer). No code change was needed; this item was already + satisfied by the `T2.15` register-project work and is now confirmed. +2. **T5.2 — `kt_sync_to_github` implemented (offline-verified 2026-09-19; + real-repo dogfood gate pending).** Acceptance: given a stored encrypted + GitHub credential, calling `kt_sync_to_github` + creates/updates a linked GitHub Issue for a **track** (per `docs/PRD.md` + §4.13 — the tool is track-scoped, `{project_id, track_id}`, with **no + item-level sync target in v1**; a prior draft of this line said "item", + which was drift against the shipped tool schema and §4.13, corrected + 2026-09-19) and records the issue URL on the track (surfaced on + `kt_get_track`'s `track.github_issue_url`), verified against one real + test repository. depends_on: `T2.12`, `T5.1`. + **Status (2026-09-19):** the create/update/no-op sync, its durable + creation-intent idempotency (a `pending` `track_external_links` row is + committed before the outbound POST, so a create-then-crash recovers via + the hidden body marker instead of duplicating), and the §4.13 + operational-error envelope (`GITHUB_AUTH_FAILED` / `GITHUB_NOT_FOUND` / + `GITHUB_RATE_LIMITED` / `GITHUB_TIMEOUT` / `GITHUB_UNKNOWN_ERROR`) are + implemented and covered by offline unit + integration tests (fake + injected `GitHubClient`, no network in CI). Design panel 2026-09-19 + (unanimous Option A: persisted link + native `fetch`, no SDK); Paul + chose the fully-hardened durable-intent variant. Two deliberate v1 + bounds, documented not hidden: retries/backoff are intentionally absent + (a transient 5xx/secondary-rate-limit surfaces as `{ok:false}` for the + caller to retry — add them here if the GitHub surface grows); and a + stale-link `404` on update returns `GITHUB_NOT_FOUND` and does **not** + auto-recreate (a relink tool would be a separate item). **Update-path + concurrency (considered + deferred, 2026-09-19):** the *create* path is + fully serialized (the `UNIQUE(track_id, adapter_type)` claim guarantees + exactly one create, no duplicate). For the *update* path we deliberately + do **not** hold a DB/advisory lock across the GitHub network call — + CodeRabbit flagged this (Major) and recommended one; we declined because + holding a lock across a multi-second network round-trip risks pool + exhaustion/stalls on a self-hosted server, and the worst case without it + is benign: two concurrent updates PATCH the same issue idempotently + (no duplicate, no lost issue), and a momentarily-stale `content_hash` + self-corrects on the next sync (one redundant PATCH). Revisit only if + real concurrent-update contention is observed; a future option is a + per-(track,adapter) advisory lock scoped to just the finalize, or an + `operation_id` compare-and-set on the hash write. The **one + remaining acceptance gate** is the real-repo verification against + `SathiaAI/KnoTrack` (dogfood) with a Paul-approved fine-grained PAT — not + part of the code/CI deliverable; see the handoff. `migrations/009_track_external_links.sql`. 3. **T5.3 — `kt_sync_to_linear` fully implemented.** Acceptance: given a stored encrypted Linear credential, calling `kt_sync_to_linear` creates/updates a linked Linear issue for a KnoTrack item and records diff --git a/docs/TEST_CASES.md b/docs/TEST_CASES.md index aa5315f..4bfedb4 100644 --- a/docs/TEST_CASES.md +++ b/docs/TEST_CASES.md @@ -417,7 +417,7 @@ section tests the *detection logic itself*, which is triggered both inline by `kt_sync_to_github(project_id, track_id) -> {ok} | {ok:false, error}` — only valid if a github adapter is configured for the project (a missing adapter is a `CONFLICT` error, not `{ok:false}`). -**This build (T2.13 stub):** a _configured_ github adapter (provisioned via `kt_register_project`) currently returns `INTERNAL_ERROR` (`github sync is not available in this build`) — the success (`GHSY-01`) and operational-failure (`GHSY-04`/`GHSY-05`) rows below describe the T5.2 behavior once external sync ships; the missing-adapter `CONFLICT` rows are current. +**This build (T5.2, implemented 2026-09-19):** a _configured_ github adapter now performs a real idempotent sync (create or update the track's GitHub Issue). Rows `GHSY-01`/`GHSY-04`/`GHSY-05` are the implemented behavior; `GHSY-14`..`GHSY-18` add idempotency, the missing-`repo` precondition, the URL-on-track read, and crash recovery. All are verified **offline** with an injected fake `GitHubClient` (no network in CI); the real-repo dogfood run against `SathiaAI/KnoTrack` with a fine-grained PAT is the one remaining acceptance gate (`docs/ROADMAP.md` T5.2). | Test ID | Tool/Area | Type | Preconditions | Input | Expected Result | |---|---|---|---|---|---| @@ -434,6 +434,12 @@ section tests the *detection logic itself*, which is triggered both inline by | GHSY-11 | kt_sync_to_github | Negative | Two projects, two tokens | Wrong-tenant token/project pairing | 404 | | GHSY-12 | kt_sync_to_github | Negative | Two projects, two tokens | Token for P2 (P2 has github adapter), `project_id=P2`, `track_id` belongs to P1 | 404 | | GHSY-13 | kt_sync_to_github | Negative | github adapter configured with credential | Successful sync call (GHSY-01) | The success response body itself contains no credential — verify separately from the general adapter sweep in §16, since a successful sync is the highest-risk path for accidentally echoing adapter config back | +| GHSY-14 | kt_sync_to_github | Positive (create + read) | github adapter with `repo` configured; track has no existing link | Valid call | Issue created (one HTTP create); a `track_external_links` row is `linked`; `kt_get_track`'s `track.github_issue_url` returns the Issue URL | +| GHSY-15 | kt_sync_to_github | Positive (idempotent no-op) | GHSY-14 already synced; track content unchanged | Same call again | `{ok:true}` with **no** second HTTP call — create-vs-update is decided from the link row and the content hash matches | +| GHSY-16 | kt_sync_to_github | Positive (update) | Linked track whose content changed | Same call | The **same** Issue is updated (no new Issue created), `{ok:true}` | +| GHSY-17 | kt_sync_to_github | Negative (precondition) | github adapter configured but with **no** `repo` | Same call | `CONFLICT` (409), message `github adapter has no repository configured`; no HTTP call | +| GHSY-18 | kt_sync_to_github | Recovery (no duplicates) | A `pending` link left by a crashed prior (ambiguous) create | Same call | Recovers via the hidden `` marker: **adopts** the existing Issue when the marker is found; on a miss it **never** auto-recreates (a Search miss can't prove the ambiguous create didn't land) — it returns an actionable error and leaves the pending link for explicit resolution. Duplicate prevention takes priority over auto-retry. | +| GHSY-19 | kt_sync_to_github | Negative (no silent retarget) | Track linked to an Issue in repo A; adapter later repointed to repo B | Same call | `{ok:false, error:"...refusing to update a different repository"}` — the sync refuses rather than update an unrelated Issue | --- diff --git a/docs/TRD.md b/docs/TRD.md index e749efc..dc75a8e 100644 --- a/docs/TRD.md +++ b/docs/TRD.md @@ -699,7 +699,7 @@ Input schema: ``` Two distinct failure surfaces, deliberately kept separate: -- **Preconditions the caller can fix by calling a different tool first** (no `github` credentials stored for this project) → a real tool-level error, `409 CONFLICT`, via the `isError` envelope (§3.1). +- **Preconditions the caller can fix by configuring the adapter first** (no `github` credentials stored for this project, or a stored `github` adapter with no `repo` in its config so no API URL can be formed) → a real tool-level error, `409 CONFLICT`, via the `isError` envelope (§3.1); no HTTP call is made. - **Everything about talking to GitHub itself** (bad token, repo not found, rate-limited, network timeout) → **not** an MCP-level error at all; the tool call succeeds and returns the discriminated result `{ok: false, error: "..."}` per the mandated signature, because these are expected, retryable operational outcomes rather than contract violations. Example success output: @@ -713,7 +713,9 @@ Example operational-failure output (still a successful tool call): ``` Other `error` string prefixes used: `GITHUB_AUTH_FAILED` (401/403 from GitHub — token revoked or insufficient scope), `GITHUB_NOT_FOUND` (repo or issue not found), `GITHUB_TIMEOUT` (exceeded `KNOTRACK_GITHUB_SYNC_TIMEOUT_MS`, default 8000ms), `GITHUB_UNKNOWN_ERROR` (anything else, with the upstream status code appended). -Errors (tool-level, via `isError`): `401`; `404` (project or track not found); `409` (no GitHub credentials configured for this project — i.e. no row in `adapters` for `(project_id, 'github')`); `422` (malformed uuid); `500` (credential decryption failure, unexpected local exception before the GitHub call was even attempted). +Errors (tool-level, via `isError`): `401`; `404` (project or track not found); `409` (no GitHub credentials configured for this project — no row in `adapters` for `(project_id, 'github')` — or a configured `github` adapter with no `repo`); `422` (malformed uuid); `500` (credential decryption failure, unexpected local exception before the GitHub call was even attempted). + +Idempotency (T5.2): the authoritative create-vs-update key is a `track_external_links` row (`migrations/009`, unique on `(track_id, adapter_type)`), never a GitHub search. A `pending` row is committed before the outbound create (durable creation-intent), so a crash mid-create recovers via a hidden `` body marker rather than duplicating. The created/updated issue URL is recorded on the track and surfaced on `kt_get_track` as `track.github_issue_url`. Retries/backoff are intentionally out of scope in v1. ### 3.15 `kt_sync_to_linear` diff --git a/migrations/009_track_external_links.down.sql b/migrations/009_track_external_links.down.sql new file mode 100644 index 0000000..374a1c7 --- /dev/null +++ b/migrations/009_track_external_links.down.sql @@ -0,0 +1,27 @@ +-- KnoTrack — 009_track_external_links.down.sql +-- Reverse of 009_track_external_links.sql (T5.2). Following the 005 down +-- convention: dropping this table is permanent loss of every track->Issue +-- link, so refuse when any row exists rather than silently discarding sync +-- state. An operator who really wants to roll back clears the table first +-- (or comments out the guard). The shared set_updated_at() function is +-- left in place — other tables use it; DROP TABLE removes this table's own +-- trigger and index automatically. + +BEGIN; + +DO $$ +BEGIN + -- Guard the row check for a re-run where the table is already gone: + -- SELECT ... FROM a missing table raises undefined_table and would abort + -- the rollback before the idempotent DROP below. + IF to_regclass('track_external_links') IS NOT NULL THEN + IF EXISTS (SELECT 1 FROM track_external_links) THEN + RAISE EXCEPTION + 'Refusing to drop track_external_links: at least one row exists (a track is linked to or mid-sync with an external issue). Back up or intentionally clear the data first if you really want to roll this back.'; + END IF; + END IF; +END $$; + +DROP TABLE IF EXISTS track_external_links; + +COMMIT; diff --git a/migrations/009_track_external_links.sql b/migrations/009_track_external_links.sql new file mode 100644 index 0000000..8808de1 --- /dev/null +++ b/migrations/009_track_external_links.sql @@ -0,0 +1,60 @@ +-- ============================================================ +-- track_external_links (docs/ROADMAP.md T5.2 / docs/PRD.md §4.13) +-- ============================================================ +-- Durable link between a KnoTrack track and the external issue it is +-- synced to (GitHub in T5.2, Linear in T5.3 via the same table). This is +-- the AUTHORITATIVE idempotency store for kt_sync_to_github: the tool +-- decides create-vs-update from a row here, never from an eventually- +-- consistent GitHub search (design panel 2026-09-19, unanimous Option A). +-- +-- sync_state carries a durable creation-intent so a crash *between* +-- GitHub accepting a create (HTTP 201) and this row being finalized never +-- silently produces a duplicate issue on the next sync: +-- 'pending' — a creation attempt was durably claimed (committed) BEFORE +-- the outbound POST. If the process dies mid-flight the row +-- stays 'pending'; the next sync sees it, refuses to blindly +-- POST again, and routes to explicit marker-based recovery. +-- 'linked' — a real issue exists; external_id/url/content_hash are set. +-- +-- external_id is the repo-scoped GitHub *issue number* (PATCH needs the +-- number, not the URL). repo_identity binds the link to the exact +-- owner/repo it was created against, so a later silent change to the +-- adapter's configured repo cannot update an unrelated issue. +-- operation_id lets a stale completion be detected and rejected rather +-- than overwriting a newer result. + +BEGIN; + +CREATE TABLE track_external_links ( + id uuid PRIMARY KEY DEFAULT gen_random_uuid(), + track_id uuid NOT NULL REFERENCES tracks (id) ON DELETE CASCADE, + adapter_type text NOT NULL CHECK (adapter_type IN ('github', 'linear')), + sync_state text NOT NULL CHECK (sync_state IN ('pending', 'linked')), + external_id text, + external_url text, + repo_identity text NOT NULL, + content_hash text, + operation_id uuid NOT NULL, + created_at timestamptz NOT NULL DEFAULT now(), + updated_at timestamptz NOT NULL DEFAULT now(), + -- at most one link of a given adapter type per track; this UNIQUE is the + -- real cross-process guard against two concurrent syncs both creating an + -- issue (INSERT ... ON CONFLICT DO NOTHING claims the slot atomically). + CONSTRAINT uq_track_external_links_track_type UNIQUE (track_id, adapter_type), + -- a linked row must carry its full identity; a pending row need not. + CONSTRAINT ck_track_external_links_linked_complete CHECK ( + sync_state <> 'linked' + OR (external_id IS NOT NULL AND external_url IS NOT NULL AND content_hash IS NOT NULL) + ) +); + +-- lookups that resolve "which track owns issue #N" during recovery. +CREATE INDEX idx_track_external_links_external_id + ON track_external_links (external_id); + +CREATE TRIGGER trg_track_external_links_set_updated_at + BEFORE UPDATE ON track_external_links + FOR EACH ROW + EXECUTE FUNCTION set_updated_at(); + +COMMIT; diff --git a/src/db/queries/adapters.ts b/src/db/queries/adapters.ts index 929fa15..4212c8d 100644 --- a/src/db/queries/adapters.ts +++ b/src/db/queries/adapters.ts @@ -63,6 +63,25 @@ export async function listAdaptersForProject( return result.rows; } +/** Single adapter row for (project, type), including `encrypted_credential` + * and `config` — used by the sync tools (kt_sync_to_github / T5.3 + * kt_sync_to_linear), which must decrypt the credential and read the + * configured `repo`. Returns undefined when no adapter of that type is + * configured. Distinct from `adapterConfigured` (existence-only, never + * selects the secret): a caller that only needs the precondition check + * must keep using that one so the secret is never read needlessly. */ +export async function getAdapterForProject( + db: Queryable, + projectId: string, + type: 'github' | 'linear', +): Promise { + const result = await db.query( + `SELECT * FROM adapters WHERE project_id = $1 AND type = $2 LIMIT 1`, + [projectId, type], + ); + return result.rows[0]; +} + /** Every adapter row across every project — used only by * scripts/rotate-encryption-key.ts, which has to re-encrypt every stored * credential regardless of which project it belongs to. No other call diff --git a/src/db/queries/track-external-links.ts b/src/db/queries/track-external-links.ts new file mode 100644 index 0000000..3bf9e27 --- /dev/null +++ b/src/db/queries/track-external-links.ts @@ -0,0 +1,161 @@ +// track_external_links access (migrations/009, docs/ROADMAP.md T5.2). +// +// The authoritative idempotency store for kt_sync_to_github. Every function +// that mutates a link expects to run inside a withTransaction client so the +// row lock / unique-conflict claim is atomic; the read used before a sync +// (`getLinkForUpdate`) takes `FOR UPDATE` and therefore also needs a +// transaction client, never a bare Pool. +import type { Pool, PoolClient } from 'pg'; + +type Queryable = Pool | PoolClient; + +export type SyncState = 'pending' | 'linked'; + +export interface TrackExternalLinkRow { + id: string; + track_id: string; + adapter_type: 'github' | 'linear'; + sync_state: SyncState; + external_id: string | null; + external_url: string | null; + repo_identity: string; + content_hash: string | null; + operation_id: string; + created_at: Date; + updated_at: Date; +} + +/** Reads the link for (track, type) and locks the row for the rest of the + * transaction, so a concurrent sync of the same track serializes behind it + * instead of both reaching the create decision. Returns undefined when no + * link exists yet. Must be called on a transaction client. */ +export async function getLinkForUpdate( + client: PoolClient, + trackId: string, + adapterType: 'github' | 'linear', +): Promise { + const result = await client.query( + `SELECT * FROM track_external_links + WHERE track_id = $1 AND adapter_type = $2 + FOR UPDATE`, + [trackId, adapterType], + ); + return result.rows[0]; +} + +/** Atomically claims a fresh `pending` creation slot. The UNIQUE(track_id, + * adapter_type) constraint makes this the real cross-process guard: if two + * syncs race, exactly one INSERT returns a row and the other gets undefined + * (its `getLinkForUpdate` re-read will then see the winner's row). The + * claim is meant to be COMMITTED before any outbound POST, so a crash mid- + * create leaves this durable `pending` marker behind. */ +export async function claimPendingLink( + client: PoolClient, + input: { + trackId: string; + adapterType: 'github' | 'linear'; + repoIdentity: string; + operationId: string; + }, +): Promise { + const result = await client.query( + `INSERT INTO track_external_links + (track_id, adapter_type, sync_state, repo_identity, operation_id) + VALUES ($1, $2, 'pending', $3, $4) + ON CONFLICT (track_id, adapter_type) DO NOTHING + RETURNING *`, + [input.trackId, input.adapterType, input.repoIdentity, input.operationId], + ); + return result.rows[0]; +} + +/** Transitions a `pending` claim (identified by its operation_id) to + * `linked`, recording the real issue identity and content hash. Returns the + * updated row, or undefined if the guard didn't match (the pending row was + * cleared or reclaimed under a different operation_id meanwhile) — the + * caller treats that as "someone else finalized" and does not retry blind. */ +export async function finalizeLinked( + client: PoolClient, + input: { + trackId: string; + adapterType: 'github' | 'linear'; + operationId: string; + externalId: string; + externalUrl: string; + contentHash: string; + }, +): Promise { + const result = await client.query( + `UPDATE track_external_links + SET sync_state = 'linked', + external_id = $3, + external_url = $4, + content_hash = $5, + repo_identity = repo_identity + WHERE track_id = $1 AND adapter_type = $2 AND operation_id = $6 + RETURNING *`, + [ + input.trackId, + input.adapterType, + input.externalId, + input.externalUrl, + input.contentHash, + input.operationId, + ], + ); + return result.rows[0]; +} + +/** Records a new content hash on an already-`linked` row after a successful + * PATCH reconcile. external_id/url are unchanged (same issue). */ +export async function updateLinkedContentHash( + client: PoolClient, + input: { + trackId: string; + adapterType: 'github' | 'linear'; + contentHash: string; + }, +): Promise { + await client.query( + `UPDATE track_external_links + SET content_hash = $3 + WHERE track_id = $1 AND adapter_type = $2 AND sync_state = 'linked'`, + [input.trackId, input.adapterType, input.contentHash], + ); +} + +/** Deletes our own `pending` claim (guarded by operation_id) so a fresh + * create can proceed. Used when a create failed in a way that proves no + * issue was created (a definitive GitHub rejection), or when marker-based + * recovery has established the issue does not exist. */ +export async function clearPendingLink( + client: PoolClient, + input: { + trackId: string; + adapterType: 'github' | 'linear'; + operationId: string; + }, +): Promise { + await client.query( + `DELETE FROM track_external_links + WHERE track_id = $1 AND adapter_type = $2 AND sync_state = 'pending' AND operation_id = $3`, + [input.trackId, input.adapterType, input.operationId], + ); +} + +/** The linked GitHub issue URL for a track, or null — surfaced on + * kt_get_track so "records the issue URL on the track" is observable + * through the item/track read contract without denormalizing the URL onto + * the tracks table. Only a `linked` row exposes a URL. */ +export async function getGithubIssueUrlForTrack( + db: Queryable, + trackId: string, +): Promise { + const result = await db.query<{ external_url: string | null }>( + `SELECT external_url FROM track_external_links + WHERE track_id = $1 AND adapter_type = 'github' AND sync_state = 'linked' + LIMIT 1`, + [trackId], + ); + return result.rows[0]?.external_url ?? null; +} diff --git a/src/db/queries/tracks.ts b/src/db/queries/tracks.ts index 893d64d..ba1544d 100644 --- a/src/db/queries/tracks.ts +++ b/src/db/queries/tracks.ts @@ -359,3 +359,26 @@ export async function trackExistsInProject( ]); return (result.rowCount ?? 0) > 0; } + +/** Stamps `tracks.last_github_sync_at` after a successful GitHub sync + * (T5.2), to the payload snapshot time `at`. The column was added in + * migrations/005 for exactly this + * "most recent successful sync" watermark; SYNC_DRIFT (T6) reads it against + * the track's most recent change. This does bump `updated_at` via the + * table's trigger — so T6's SYNC_DRIFT must compare this watermark against + * item-change timestamps (getMaxItemUpdatedAtByTrackIds), NOT + * tracks.updated_at, or it would never see drift (the sync would look like + * the most recent change to the track). A completed sync is a real event on + * the track, so bumping updated_at is acceptable here (unlike the 006 + * backfill, which suppressed the trigger because it changed nothing real). */ +export async function touchGithubSyncWatermark( + db: Queryable, + trackId: string, + at: Date, +): Promise { + // `at` is the payload SNAPSHOT time, not the completion time: an item that + // changed after the snapshot but before this write must still read as drift + // (updated_at > last_github_sync_at), so we stamp the state we actually + // pushed, not the later "now()". + await db.query(`UPDATE tracks SET last_github_sync_at = $2 WHERE id = $1`, [trackId, at]); +} diff --git a/src/github/github-client.ts b/src/github/github-client.ts new file mode 100644 index 0000000..4c7c4e3 --- /dev/null +++ b/src/github/github-client.ts @@ -0,0 +1,249 @@ +// GitHub REST client for kt_sync_to_github (docs/PRD.md §4.13, T5.2). +// +// Deliberately a thin native-fetch wrapper, not @octokit/* — the design +// panel (2026-09-19, unanimous) chose the smallest auditable dependency +// surface for a self-hosted server, since this is two write endpoints plus +// one recovery search. Every method makes exactly one HTTP call and +// returns a discriminated result; it NEVER throws for an HTTP-level outcome +// and NEVER logs or echoes the token. +// +// Operational failures are mapped to the fixed PRD §4.13 prefixes +// (GITHUB_AUTH_FAILED / GITHUB_NOT_FOUND / GITHUB_RATE_LIMITED / +// GITHUB_TIMEOUT / GITHUB_UNKNOWN_ERROR). `ambiguous` tells the caller +// whether a create request may have reached GitHub and created an issue +// despite the failure (timeout / 5xx / network) — the caller uses it to +// decide whether a durable `pending` link is safe to clear. + +const GITHUB_API = 'https://api.github.com'; + +export interface GitHubIssuePayload { + title: string; + body: string; + state: 'open' | 'closed'; + state_reason?: 'completed'; +} + +export interface GitHubIssueRef { + /** repo-scoped issue number, as a string (PATCH targets this, not the URL). */ + number: string; + html_url: string; +} + +export type GitHubResult = + { ok: true; value: T } | { ok: false; error: string; ambiguous: boolean }; + +export interface GitHubClient { + /** POST /repos/{repo}/issues — creates an (open) issue with title+body. */ + createIssue(repo: string, payload: GitHubIssuePayload): Promise>; + /** PATCH /repos/{repo}/issues/{number} — reconciles title/body/state. */ + updateIssue( + repo: string, + issueNumber: string, + payload: GitHubIssuePayload, + ): Promise>; + /** Recovery-only: find an existing issue whose body carries `marker`. + * Returns null when none is found. */ + findIssueByMarker(repo: string, marker: string): Promise>; +} + +export interface GitHubClientOptions { + timeoutMs: number; + userAgent: string; +} + +export type GitHubClientFactory = (token: string, opts: GitHubClientOptions) => GitHubClient; + +function isRateLimited(status: number, headers: Headers, bodyText: string): boolean { + if (status === 429) return true; + if (status === 403) { + if (headers.get('retry-after')) return true; + if (headers.get('x-ratelimit-remaining') === '0') return true; + // Secondary rate limits return 403 with a body message rather than the + // primary-limit headers — treat those as rate-limited (retryable), not + // as an auth failure. + if (/secondary rate limit|\brate limit\b/i.test(bodyText)) return true; + } + return false; +} + +/** Maps a completed HTTP response (non-2xx) to a prefixed error string plus + * the ambiguity flag. A 4xx means GitHub rejected the request, so a create + * that got one did NOT create an issue (ambiguous=false). A 5xx is + * different: GitHub may have created the issue and then failed to respond + * cleanly, so a create that got a 5xx is AMBIGUOUS — the caller must keep + * its pending link for marker recovery rather than recreating. Timeouts and + * network errors are handled as ambiguous by the caller's catch. */ +function mapErrorResponse( + status: number, + headers: Headers, + bodyText: string, +): { + error: string; + ambiguous: boolean; +} { + const detail = bodyText.slice(0, 300).replace(/\s+/g, ' ').trim(); + if (status === 401) + return { error: `GITHUB_AUTH_FAILED: ${detail || 'unauthorized'}`, ambiguous: false }; + if (isRateLimited(status, headers, bodyText)) + return { error: `GITHUB_RATE_LIMITED: ${detail || 'rate limit exceeded'}`, ambiguous: false }; + if (status === 403) + return { error: `GITHUB_AUTH_FAILED: ${detail || 'forbidden'}`, ambiguous: false }; + if (status === 404) + return { error: `GITHUB_NOT_FOUND: ${detail || 'not found'}`, ambiguous: false }; + // No dedicated validation prefix in the PRD's closed set; 422 and any + // other status fold into UNKNOWN with GitHub's own message. 5xx is + // ambiguous for the create path (the write may have landed server-side). + return { + error: `GITHUB_UNKNOWN_ERROR: HTTP ${status}${detail ? `: ${detail}` : ''}`, + ambiguous: status >= 500, + }; +} + +function mapThrown(err: unknown): { error: string; ambiguous: boolean } { + if (err instanceof Error && err.name === 'AbortError') { + return { error: 'GITHUB_TIMEOUT: request exceeded the configured timeout', ambiguous: true }; + } + const msg = err instanceof Error ? err.message : String(err); + // A request that never got a response may still have been received and + // acted on by GitHub -> ambiguous for the create path. + return { error: `GITHUB_UNKNOWN_ERROR: ${msg}`, ambiguous: true }; +} + +export function createFetchGitHubClient(token: string, opts: GitHubClientOptions): GitHubClient { + const headers: Record = { + Authorization: `Bearer ${token}`, + Accept: 'application/vnd.github+json', + 'X-GitHub-Api-Version': '2022-11-28', + 'User-Agent': opts.userAgent, + 'Content-Type': 'application/json', + }; + + async function send( + method: 'GET' | 'POST' | 'PATCH', + url: string, + jsonBody?: unknown, + ): Promise> { + const controller = new AbortController(); + const timer = setTimeout(() => controller.abort(), opts.timeoutMs); + try { + const res = await fetch(url, { + method, + headers, + body: jsonBody === undefined ? undefined : JSON.stringify(jsonBody), + signal: controller.signal, + }); + const text = await res.text(); + if (res.ok) { + let body: unknown = undefined; + try { + body = text.length > 0 ? JSON.parse(text) : undefined; + } catch { + return { + ok: false, + error: 'GITHUB_UNKNOWN_ERROR: malformed JSON in GitHub response', + // A 2xx WRITE (POST/PATCH) may already have created/changed the + // issue, so keep it ambiguous — the caller retains its pending + // link for recovery instead of clearing and recreating a + // duplicate. A GET (the recovery search) creates nothing, so a + // malformed read is not ambiguous. + ambiguous: method !== 'GET', + }; + } + return { ok: true, value: { status: res.status, body } }; + } + const mapped = mapErrorResponse(res.status, res.headers, text); + return { ok: false, error: mapped.error, ambiguous: mapped.ambiguous }; + } catch (err) { + const mapped = mapThrown(err); + return { ok: false, error: mapped.error, ambiguous: mapped.ambiguous }; + } finally { + clearTimeout(timer); + } + } + + function toRef(body: unknown): GitHubIssueRef | null { + if (body === null || typeof body !== 'object') return null; + const rec = body as Record; + const num = rec.number; + const url = rec.html_url; + if (typeof num !== 'number' || typeof url !== 'string') return null; + return { number: String(num), html_url: url }; + } + + return { + async createIssue(repo, payload) { + const res = await send('POST', `${GITHUB_API}/repos/${repo}/issues`, { + title: payload.title, + body: payload.body, + }); + if (!res.ok) return res; + const ref = toRef(res.value.body); + if (!ref) + return { + ok: false, + error: 'GITHUB_UNKNOWN_ERROR: created issue response missing number/html_url', + // The issue was in fact created (2xx); treat as ambiguous so the + // caller keeps the pending link for marker recovery rather than + // recreating. + ambiguous: true, + }; + return { ok: true, value: ref }; + }, + + async updateIssue(repo, issueNumber, payload) { + const patch: Record = { + title: payload.title, + body: payload.body, + state: payload.state, + }; + if (payload.state_reason) patch.state_reason = payload.state_reason; + const res = await send('PATCH', `${GITHUB_API}/repos/${repo}/issues/${issueNumber}`, patch); + if (!res.ok) return res; + const ref = toRef(res.value.body); + if (!ref) + return { + ok: false, + error: 'GITHUB_UNKNOWN_ERROR: updated issue response missing number/html_url', + ambiguous: false, + }; + return { ok: true, value: ref }; + }, + + async findIssueByMarker(repo, marker) { + // Recovery-only. `type:issue` excludes pull requests (the search API + // returns both). GitHub search is eventually consistent AND can return + // incomplete results, so a null is never immediate proof no issue + // exists — the caller pairs it with the pending link's age, and an + // INCOMPLETE search is reported as an error so recovery does not + // recreate on a false miss. + const q = encodeURIComponent(`repo:${repo} type:issue in:body "${marker}"`); + const res = await send('GET', `${GITHUB_API}/search/issues?q=${q}&per_page=20`); + if (!res.ok) return res; + const body = (res.value.body ?? {}) as Record; + const items = Array.isArray(body.items) ? (body.items as unknown[]) : []; + for (const item of items) { + if (item && typeof item === 'object') { + const rec = item as Record; + if (typeof rec.body === 'string' && rec.body.includes(marker)) { + const ref = toRef(item); + if (ref) return { ok: true, value: ref }; + } + } + } + // No match found. Only trust that as "no issue exists" if the search + // was complete; otherwise report it unconfirmed so the caller retries + // instead of recreating a possibly-existing issue. + const incomplete = body.incomplete_results === true; + const total = typeof body.total_count === 'number' ? body.total_count : items.length; + if (incomplete || total > items.length) { + return { + ok: false, + error: + 'GITHUB_UNKNOWN_ERROR: issue search returned incomplete results; could not confirm', + ambiguous: true, + }; + } + return { ok: true, value: null }; + }, + }; +} diff --git a/src/mcp/tools/get-track.ts b/src/mcp/tools/get-track.ts index 9019918..0a1328d 100644 --- a/src/mcp/tools/get-track.ts +++ b/src/mcp/tools/get-track.ts @@ -6,6 +6,7 @@ import { getTrackInputSchema, type GetTrackInput } from '../../schemas/tools.js' import { findActiveProjectById } from '../../db/queries/projects.js'; import { findTrackById, getDependsOnTrackIds } from '../../db/queries/tracks.js'; import { getItemDependencyEdgesForTrack, listItemsByTrack } from '../../db/queries/items.js'; +import { getGithubIssueUrlForTrack } from '../../db/queries/track-external-links.js'; import { withReadSnapshot } from '../../db/tx.js'; import { notFound } from '../errors.js'; import { runTool } from '../tool-helpers.js'; @@ -22,6 +23,10 @@ export interface GetTrackOutput extends Record { pivot_decision_id: string | null; source_doc_ref: string | null; depends_on_track_ids: string[]; + /** T5.2: the URL of the GitHub Issue this track is synced to, or null. + * This is where "kt_sync_to_github records the issue URL on the track" + * (docs/PRD.md §4.13) is observable through the read contract. */ + github_issue_url: string | null; created_at: string; }; items: Array<{ @@ -61,6 +66,7 @@ export async function getTrackService( const dependsOnTrackIds = await getDependsOnTrackIds(client, track.id); const items = await listItemsByTrack(client, track.id); const itemEdges = await getItemDependencyEdgesForTrack(client, track.id); + const githubIssueUrl = await getGithubIssueUrlForTrack(client, track.id); const dependsOnByItemId = new Map(); for (const edge of itemEdges) { @@ -80,6 +86,7 @@ export async function getTrackService( pivot_decision_id: track.pivot_decision_id, source_doc_ref: track.source_doc_ref, depends_on_track_ids: dependsOnTrackIds, + github_issue_url: githubIssueUrl, created_at: track.created_at.toISOString(), }, items: items.map((item) => ({ diff --git a/src/mcp/tools/issue-payload.ts b/src/mcp/tools/issue-payload.ts new file mode 100644 index 0000000..352b3a7 --- /dev/null +++ b/src/mcp/tools/issue-payload.ts @@ -0,0 +1,152 @@ +// Pure track -> GitHub Issue payload mapping + content hashing for +// kt_sync_to_github (docs/PRD.md §4.13, docs/ROADMAP.md T5.2). +// +// Kept side-effect-free and free of any DB/HTTP import so the mapping and +// the no-op-detection hash are unit-testable in isolation. KnoTrack owns +// exactly three fields on the issue — title, body, state — and nothing +// else (no labels/assignees/milestones); the body carries a hidden marker +// used ONLY for crash recovery, never as the authoritative create-vs-update +// key (that is the track_external_links row). +import { createHash } from 'node:crypto'; + +// GitHub hard limits we stay under rather than letting the API 422. +const TITLE_MAX = 256; +const BODY_MAX = 65_536; +// A very large track would otherwise render an unbounded checklist into the +// body; cap the rendered items and note the elision. Generous vs a real +// track's item count while bounding worst-case body size. +const ITEM_RENDER_CAP = 300; + +export interface TrackForIssue { + id: string; + title: string; + /** Derived track status (T2.16): on_track | pivot_pending | blocked | done. */ + status: string; +} + +export interface ItemForIssue { + /** Stable tie-breaker for equal sequence_position (which the schema + * permits) so the rendered order — and therefore the content hash — is + * deterministic across calls. */ + id: string; + title: string; + /** Item status: pending | in_progress | done | blocked. */ + status: string; + sequence_position: number; +} + +export interface IssuePayload { + title: string; + body: string; + state: 'open' | 'closed'; + /** Only set when closing, so GitHub records *why*. */ + state_reason?: 'completed'; +} + +/** The hidden HTML comment embedded in the issue body. Recovery-only: after + * a crash that left a `pending` link, the sync tool searches for this exact + * string to decide whether the create actually happened. Never parsed as + * the normal idempotency key. */ +export function trackMarker(trackId: string): string { + return ``; +} + +function truncate(value: string, max: number): string { + return value.length <= max ? value : value.slice(0, max); +} + +function itemCheckbox(status: string): string { + return status === 'done' ? '[x]' : '[ ]'; +} + +/** Neutralizes HTML-comment delimiters in user-controlled text (item/track + * titles) so a title can never inject a fake `` + * marker into the body — which would let recovery search match or overwrite + * an unrelated issue — nor open a stray hidden HTML comment. HTML-escaping + * the delimiters keeps the text readable (GitHub renders `<`/`>` as + * literal `<`/`>`) while making the marker syntax impossible to form. */ +function sanitize(value: string): string { + return value.replace(//g, '-->'); +} + +/** Deterministic mapping — pure function of (track, items). `done` is the + * only terminal item/track status in the model, so a `done` track closes + * its issue and everything else leaves it open. Items are rendered in + * sequence order for a stable body (and therefore a stable content hash). */ +export function buildIssuePayload(track: TrackForIssue, items: ItemForIssue[]): IssuePayload { + // Escape FIRST, then enforce the limit: sanitize expands `` into + // longer entities, so truncating before escaping could push the result back + // over GitHub's 256-char title cap and 422 the sync. + const title = truncate(sanitize(track.title), TITLE_MAX); + const isDone = track.status === 'done'; + + // Deterministic order: sequence_position, then id as a stable tie-breaker + // (sequence_position is NOT unique per track), so an unchanged track always + // renders the same body — and hashes the same — across calls. + const ordered = [...items].sort( + (a, b) => a.sequence_position - b.sequence_position || (a.id < b.id ? -1 : a.id > b.id ? 1 : 0), + ); + + const header = [ + '_Synced from KnoTrack — KnoTrack owns this issue’s title, body, and open/closed state and will overwrite manual edits to them._', + '', + `**Track status:** \`${track.status}\``, + '', + '### Items', + '', + ].join('\n'); + + // The hidden recovery marker MUST survive truncation (losing it would break + // crash recovery -> duplicates), and the checklist is built WITHIN the + // remaining byte budget so a long body is never silently cut without an + // accurate "N more not shown" notice — which also changes the hash, forcing + // a re-sync rather than a silent partial. Reserve room for the marker and a + // worst-case notice. + const markerBlock = `\n\n${trackMarker(track.id)}`; + const NOTICE_RESERVE = 64; + const budget = Math.max(0, BODY_MAX - markerBlock.length); + + const lines: string[] = []; + let used = header.length; + let shownCount = 0; + for (const item of ordered) { + if (shownCount >= ITEM_RENDER_CAP) break; + const line = `- ${itemCheckbox(item.status)} ${sanitize(item.title)}\n`; + const reserve = shownCount + 1 < ordered.length ? NOTICE_RESERVE : 0; + if (used + line.length + reserve > budget) break; + lines.push(line); + used += line.length; + shownCount += 1; + } + + const omitted = ordered.length - shownCount; + const checklist = + shownCount > 0 ? lines.join('') : ordered.length === 0 ? '_No items yet._\n' : ''; + const notice = omitted > 0 ? `\n_… ${omitted} more item(s) not shown._` : ''; + + const body = truncate(header + checklist + notice, budget) + markerBlock; + + const payload: IssuePayload = { + title, + body, + state: isDone ? 'closed' : 'open', + }; + if (isDone) { + payload.state_reason = 'completed'; + } + return payload; +} + +/** sha256 of the canonical, ordered representation of exactly the fields + * KnoTrack pushes. A re-sync whose payload hashes to the stored value is a + * no-op (no HTTP call). Note: this detects KnoTrack-side changes only — it + * cannot detect an issue edited, deleted, or made inaccessible on GitHub. */ +export function payloadContentHash(payload: IssuePayload): string { + const canonical = JSON.stringify({ + title: payload.title, + body: payload.body, + state: payload.state, + state_reason: payload.state_reason ?? null, + }); + return createHash('sha256').update(canonical, 'utf8').digest('hex'); +} diff --git a/src/mcp/tools/sync-to-github.ts b/src/mcp/tools/sync-to-github.ts index 466c122..3b8e236 100644 --- a/src/mcp/tools/sync-to-github.ts +++ b/src/mcp/tools/sync-to-github.ts @@ -1,17 +1,366 @@ -// kt_sync_to_github — docs/PRD.md §4.13 (T2.13 stub slice). +// kt_sync_to_github — docs/PRD.md §4.13, docs/ROADMAP.md T5.2. +// +// Syncs ONE track (not an item — §4.13: "no item-level sync target in v1") +// to a GitHub Issue, idempotently. The authoritative create-vs-update key +// is a track_external_links row, never a GitHub search (design panel +// 2026-09-19, unanimous Option A). Idempotency is hardened with a durable +// creation-intent: a `pending` link is committed BEFORE the outbound POST, +// so a crash between GitHub accepting a create and this server finalizing +// the link never silently produces a duplicate — the next call sees +// `pending` and recovers via the hidden body marker instead of blindly +// re-creating. +// +// Failure surfaces, per §4.13: +// - Missing/unusable adapter precondition -> CONFLICT (a thrown KtError). +// - Everything about actually talking to GitHub -> a SUCCESSFUL tool call +// returning { ok: false, error: ": ..." } with a fixed prefix +// (GITHUB_AUTH_FAILED / GITHUB_NOT_FOUND / GITHUB_RATE_LIMITED / +// GITHUB_TIMEOUT / GITHUB_UNKNOWN_ERROR). +import { randomUUID } from 'node:crypto'; import type { Pool } from 'pg'; import type { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'; import type { Config } from '../../config/env.js'; import { syncToGithubInputSchema, type SyncToGithubInput } from '../../schemas/tools.js'; import { runTool } from '../tool-helpers.js'; -import { syncAdapterStub, type SyncOutput } from './sync-shared.js'; +import { conflict, notFound } from '../errors.js'; +import { findActiveProjectById } from '../../db/queries/projects.js'; +import { findTrackById, touchGithubSyncWatermark } from '../../db/queries/tracks.js'; +import { listItemsByTrack } from '../../db/queries/items.js'; +import { getAdapterForProject } from '../../db/queries/adapters.js'; +import { decryptCredential } from '../../crypto/credential-cipher.js'; +import { buildIssuePayload, payloadContentHash, trackMarker } from './issue-payload.js'; +import { withReadSnapshot, withTransaction } from '../../db/tx.js'; +import { + claimPendingLink, + clearPendingLink, + finalizeLinked, + getLinkForUpdate, + updateLinkedContentHash, + type TrackExternalLinkRow, +} from '../../db/queries/track-external-links.js'; +import { + createFetchGitHubClient, + type GitHubClient, + type GitHubClientFactory, + type GitHubIssuePayload, +} from '../../github/github-client.js'; + +const ADAPTER = 'github' as const; +const USER_AGENT = 'knotrack-mcp-server'; + +export interface GithubSyncOutput extends Record { + ok: boolean; + error?: string; +} + +export interface SyncToGithubDeps { + githubClientFactory?: GitHubClientFactory; +} export async function syncToGithubService( pool: Pool, - _config: Config, + config: Config, input: SyncToGithubInput, -): Promise { - return syncAdapterStub(pool, 'github', input); + deps: SyncToGithubDeps = {}, +): Promise { + const clientFactory = deps.githubClientFactory ?? createFetchGitHubClient; + + // 1. Project + project-scoped track existence (a track_id from another + // project is a 404, never a cross-project leak). + const project = await findActiveProjectById(pool, input.project_id); + if (!project) { + throw notFound('project not found', { project_id: input.project_id }); + } + const track = await findTrackById(pool, input.project_id, input.track_id); + if (!track) { + throw notFound('track not found', { + project_id: input.project_id, + track_id: input.track_id, + }); + } + + // 2. Adapter precondition -> CONFLICT (no HTTP). §4.13. + const adapter = await getAdapterForProject(pool, input.project_id, ADAPTER); + if (!adapter) { + throw conflict('github adapter not configured', { + project_id: input.project_id, + adapter: ADAPTER, + }); + } + const repoRaw = adapter.config['repo']; + const repo = typeof repoRaw === 'string' ? repoRaw.trim() : ''; + if (!repo) { + // An adapter with no target repo can't form an API URL; it's a + // configuration precondition the caller fixes by re-registering with a + // repo, so it's a CONFLICT — not a false GitHub call. + throw conflict('github adapter has no repository configured', { + project_id: input.project_id, + adapter: ADAPTER, + }); + } + + // 3. Decrypt the stored credential (a failure here is unexpected and + // surfaces as INTERNAL_ERROR via runTool — never leaks the token). + const token = decryptCredential(adapter.encrypted_credential, config.encryptionKey); + + // 4. Deterministic payload + no-op hash. Read the track and its items in + // ONE consistent snapshot so the rendered payload (and its hash) can't + // mix a pre- and post-change view if items change mid-call. + const snap = await withReadSnapshot(pool, async (c) => { + // now() inside a REPEATABLE READ transaction is the snapshot boundary — + // the exact "as of" time of the state we are about to push. + const asOf = (await c.query<{ t: Date }>('SELECT now() AS t')).rows[0]!.t; + const t = await findTrackById(c, input.project_id, input.track_id); + return { asOf, track: t, items: t ? await listItemsByTrack(c, t.id) : [] }; + }); + if (!snap.track) { + // Track deleted between the existence check and here — treat as not found. + throw notFound('track not found', { + project_id: input.project_id, + track_id: input.track_id, + }); + } + const syncedAt = snap.asOf; + const payload = buildIssuePayload( + { id: snap.track.id, title: snap.track.title, status: snap.track.status }, + snap.items.map((i) => ({ + id: i.id, + title: i.title, + status: i.status, + sequence_position: i.sequence_position, + })), + ); + const hash = payloadContentHash(payload); + + const client = clientFactory(token, { + timeoutMs: config.githubSyncTimeoutMs, + userAgent: USER_AGENT, + }); + + // 5. Decide under a row lock; claim a fresh pending slot if none exists. + // This transaction commits before any POST, so a create's durable + // intent survives a crash. + const decision = await withTransaction(pool, async (c): Promise => { + const existing = await getLinkForUpdate(c, track.id, ADAPTER); + if (existing) { + if (existing.sync_state === 'pending') return { kind: 'pending', row: existing }; + if (existing.repo_identity !== repo) return { kind: 'repo_changed', row: existing }; + if (existing.content_hash === hash) return { kind: 'noop' }; + return { kind: 'update', row: existing }; + } + const operationId = randomUUID(); + const claimed = await claimPendingLink(c, { + trackId: track.id, + adapterType: ADAPTER, + repoIdentity: repo, + operationId, + }); + if (claimed) return { kind: 'create', operationId }; + return { kind: 'raced' }; + }); + + switch (decision.kind) { + case 'noop': + // Nothing to push, but we confirmed the issue is in sync -> watermark. + return okSynced(pool, track.id, syncedAt); + case 'repo_changed': + return { + ok: false, + error: `GITHUB_UNKNOWN_ERROR: this track is linked to an issue in ${decision.row.repo_identity}, but the adapter is now configured for ${repo}; refusing to update a different repository`, + }; + case 'raced': + return { + ok: false, + error: 'GITHUB_UNKNOWN_ERROR: another sync for this track is in progress; retry shortly', + }; + case 'update': + return updateExisting(pool, client, repo, track.id, decision.row, payload, hash, syncedAt); + case 'create': + return createFresh( + pool, + client, + repo, + track.id, + decision.operationId, + payload, + hash, + syncedAt, + ); + case 'pending': + return recoverPending(pool, client, repo, track.id, decision.row, payload, hash, syncedAt); + } +} + +/** Records the sync watermark (to the payload snapshot time) and returns + * success. Called on every ok:true path (create, update, no-op, adopt) so + * tracks.last_github_sync_at reflects the state actually pushed (T5.2; + * SYNC_DRIFT input for T6). */ +async function okSynced(pool: Pool, trackId: string, syncedAt: Date): Promise { + await withTransaction(pool, (c) => touchGithubSyncWatermark(c, trackId, syncedAt)); + return { ok: true }; +} + +type Decision = + | { kind: 'noop' } + | { kind: 'repo_changed'; row: TrackExternalLinkRow } + | { kind: 'raced' } + | { kind: 'update'; row: TrackExternalLinkRow } + | { kind: 'create'; operationId: string } + | { kind: 'pending'; row: TrackExternalLinkRow }; + +async function createFresh( + pool: Pool, + client: GitHubClient, + repo: string, + trackId: string, + operationId: string, + payload: GitHubIssuePayload, + hash: string, + syncedAt: Date, +): Promise { + const created = await client.createIssue(repo, payload); + if (!created.ok) { + if (!created.ambiguous) { + // Definitive rejection -> no issue was created; clear the pending + // claim so the next attempt starts clean instead of wedging. + await withTransaction(pool, (c) => + clearPendingLink(c, { trackId, adapterType: ADAPTER, operationId }), + ); + } + return { ok: false, error: created.error }; + } + let ref = created.value; + if (payload.state === 'closed') { + // Create makes an open issue; a done track needs a follow-up close. If + // it fails, the issue exists but the link stays pending — recovery will + // adopt and reconcile it next time (never a duplicate). + const closed = await client.updateIssue(repo, ref.number, payload); + if (!closed.ok) return { ok: false, error: closed.error }; + ref = closed.value; + } + const finalized = await withTransaction(pool, (c) => + finalizeLinked(c, { + trackId, + adapterType: ADAPTER, + operationId, + externalId: ref.number, + externalUrl: ref.html_url, + contentHash: hash, + }), + ); + if (!finalized) { + return { + ok: false, + error: + 'GITHUB_UNKNOWN_ERROR: issue created but its link could not be finalized; reconcile manually', + }; + } + return okSynced(pool, trackId, syncedAt); +} + +async function updateExisting( + pool: Pool, + client: GitHubClient, + repo: string, + trackId: string, + row: TrackExternalLinkRow, + payload: GitHubIssuePayload, + hash: string, + syncedAt: Date, +): Promise { + // row is linked -> external_id is the issue number. + const issueNumber = row.external_id; + if (!issueNumber) { + return { + ok: false, + error: 'GITHUB_UNKNOWN_ERROR: linked row is missing its issue number; reconcile manually', + }; + } + // DELIBERATE: we do NOT hold a DB/advisory lock across this network call. + // The row lock in the decision transaction is released before we get here, + // so two concurrent updates of the same track can both PATCH. That is safe: + // PATCH is idempotent, there is no duplicate/lost issue, and a momentarily- + // stale content_hash self-corrects on the next sync (one redundant PATCH). + // Holding a lock across a multi-second GitHub round-trip would risk pool + // exhaustion on a self-hosted server — a worse tradeoff. (CodeRabbit flagged + // this Major and wanted a lock; considered + deferred — see ROADMAP T5.2.) + const res = await client.updateIssue(repo, issueNumber, payload); + if (!res.ok) { + // A 404 here means the linked issue was deleted/transferred: return the + // operational error and do NOT auto-recreate in T5.2 (would risk a + // duplicate); a later relink tool can handle re-creation deliberately. + return { ok: false, error: res.error }; + } + await withTransaction(pool, (c) => + updateLinkedContentHash(c, { trackId, adapterType: ADAPTER, contentHash: hash }), + ); + return okSynced(pool, trackId, syncedAt); +} + +async function recoverPending( + pool: Pool, + client: GitHubClient, + repo: string, + trackId: string, + row: TrackExternalLinkRow, + payload: GitHubIssuePayload, + hash: string, + syncedAt: Date, +): Promise { + // If the adapter's repo changed while this sync was pending, the marker + // would be searched in (and any recreate would land in) the wrong repo. + // Refuse until the repository change is resolved. + if (row.repo_identity !== repo) { + return { + ok: false, + error: `GITHUB_UNKNOWN_ERROR: a sync is pending for this track against ${row.repo_identity}, but the adapter is now configured for ${repo}; resolve the repository change before retrying`, + }; + } + const found = await client.findIssueByMarker(repo, trackMarker(trackId)); + if (!found.ok) { + // Couldn't even run the recovery search -> stay pending, ask to retry. + return { ok: false, error: found.error }; + } + if (found.value) { + // The earlier create DID happen. Reconcile the found issue to current + // content (this also confirms it still exists), then adopt it as linked. + const ref = found.value; + const res = await client.updateIssue(repo, ref.number, payload); + if (!res.ok) return { ok: false, error: res.error }; + const finalized = await withTransaction(pool, (c) => + finalizeLinked(c, { + trackId, + adapterType: ADAPTER, + operationId: row.operation_id, + externalId: res.value.number, + externalUrl: res.value.html_url, + contentHash: hash, + }), + ); + if (!finalized) { + return { + ok: false, + error: 'GITHUB_UNKNOWN_ERROR: adopted issue could not be finalized; reconcile manually', + }; + } + return okSynced(pool, trackId, syncedAt); + } + + // Marker not found. A pending link only survives an AMBIGUOUS create + // (definitive failures clear it in createFresh), so the create outcome is + // unknown by construction here. A GitHub Search miss — even a complete one + // — does NOT prove no issue was created: search indexing is eventually + // consistent and can lag arbitrarily. Per the T5.2 design decision + // (Paul, 2026-09-19: duplicate prevention takes priority over automatic + // retry when the outcome cannot be established), we do NOT auto-recreate. + // Recovery only ever adopts an issue the marker positively finds; an + // unresolved pending link is surfaced for explicit resolution (an operator + // clears it, or a future relink tool) rather than risking a duplicate. + return { + ok: false, + error: + 'GITHUB_UNKNOWN_ERROR: a previous sync did not complete and no matching issue could be found; its outcome cannot be safely confirmed, so this track is not auto-recreated (that would risk a duplicate). Clear the pending link (or use a relink tool) to resync.', + }; } export function registerSyncToGithubTool( @@ -25,7 +374,7 @@ export function registerSyncToGithubTool( { title: 'Sync to GitHub', description: - 'Pushes a track to a linked GitHub Issue. Requires a configured GitHub adapter; external sync is not available in this build.', + 'Creates or updates a GitHub Issue for a track (idempotent). Requires a configured GitHub adapter with a repository; operational GitHub failures return { ok: false, error } rather than throwing.', inputSchema: syncToGithubInputSchema, }, async (rawArgs: unknown) => { diff --git a/tests/integration/sync-to-github.test.ts b/tests/integration/sync-to-github.test.ts index 5539637..70663af 100644 --- a/tests/integration/sync-to-github.test.ts +++ b/tests/integration/sync-to-github.test.ts @@ -1,42 +1,106 @@ +import { randomUUID } from 'node:crypto'; import { afterAll, beforeEach, describe, expect, it } from 'vitest'; -import { syncToGithubService } from '../../src/mcp/tools/sync-to-github.js'; +import { syncToGithubService, type SyncToGithubDeps } from '../../src/mcp/tools/sync-to-github.js'; +import { getTrackService } from '../../src/mcp/tools/get-track.js'; import { registerProjectService } from '../../src/mcp/tools/register-project.js'; import { createTrackService } from '../../src/mcp/tools/create-track.js'; +import { createItemService } from '../../src/mcp/tools/create-item.js'; +import { updateItemStatusService } from '../../src/mcp/tools/update-item-status.js'; import { upsertAdapter } from '../../src/db/queries/adapters.js'; +import { encryptCredential } from '../../src/crypto/credential-cipher.js'; +import type { GitHubClient, GitHubIssueRef, GitHubResult } from '../../src/github/github-client.js'; import { closeTestPool, getTestConfig, getTestPool, truncateAll, UNKNOWN_UUID } from './helpers.js'; const pool = getTestPool(); const config = getTestConfig(); +const REPO = 'SathiaAI/KnoTrack'; -async function makeProjectAndTrack(): Promise<{ project_id: string; track_id: string }> { +// ---- fake GitHub client ----------------------------------------------- +interface FakeState { + createCalls: Array<{ repo: string; payload: { state: string } }>; + updateCalls: Array<{ repo: string; number: string; payload: { state: string } }>; + findCalls: Array<{ repo: string; marker: string }>; + nextNumber: number; +} +interface FakeOverrides { + createIssue?: (s: FakeState) => GitHubResult; + updateIssue?: (s: FakeState, num: string) => GitHubResult; + findIssueByMarker?: (s: FakeState) => GitHubResult; +} +function makeFake(overrides: FakeOverrides = {}) { + const state: FakeState = { createCalls: [], updateCalls: [], findCalls: [], nextNumber: 1 }; + const client: GitHubClient = { + createIssue(repo, payload) { + state.createCalls.push({ repo, payload }); + const r: GitHubResult = overrides.createIssue + ? overrides.createIssue(state) + : { + ok: true, + value: { + number: String(state.nextNumber++), + html_url: `https://github.com/${repo}/issues/${state.nextNumber - 1}`, + }, + }; + return Promise.resolve(r); + }, + updateIssue(repo, number, payload) { + state.updateCalls.push({ repo, number, payload }); + const r: GitHubResult = overrides.updateIssue + ? overrides.updateIssue(state, number) + : { ok: true, value: { number, html_url: `https://github.com/${repo}/issues/${number}` } }; + return Promise.resolve(r); + }, + findIssueByMarker(repo, marker) { + state.findCalls.push({ repo, marker }); + const r: GitHubResult = overrides.findIssueByMarker + ? overrides.findIssueByMarker(state) + : { ok: true, value: null }; + return Promise.resolve(r); + }, + }; + const deps: SyncToGithubDeps = { githubClientFactory: () => client }; + return { state, deps }; +} + +async function makeProjectTrack(opts: { repo?: string | undefined; withGithub?: boolean } = {}) { + const withGithub = opts.withGithub ?? true; + const adapters = withGithub + ? { github: { personal_access_token: 'ghp_test_secret', repo: opts.repo } } + : undefined; const { project_id } = await registerProjectService(pool, config, { name: 'P', source_type: 'local', - source_ref: `/tmp/${crypto.randomUUID()}`, - adapters: undefined, + source_ref: `/tmp/${randomUUID()}`, + adapters, }); const { track_id } = await createTrackService(pool, config, { project_id, - title: 'T', + title: 'Auth overhaul', depends_on: [], source_doc_ref: undefined, }); return { project_id, track_id }; } +async function linkRow(trackId: string) { + const r = await pool.query( + `SELECT * FROM track_external_links WHERE track_id = $1 AND adapter_type = 'github'`, + [trackId], + ); + return r.rows[0] as + { sync_state: string; external_id: string | null; external_url: string | null } | undefined; +} + beforeEach(async () => { await truncateAll(); }); - afterAll(async () => { await closeTestPool(); }); -describe('kt_sync_to_github (T2.13 stub)', () => { - // TEST_CASES GHSY-03: a linear adapter is present but no github one — - // the error must name github specifically, not fail generically. - it('negative: CONFLICT (github-specific) when no github adapter is configured, even with a linear adapter present', async () => { - const { project_id, track_id } = await makeProjectAndTrack(); +describe('kt_sync_to_github — preconditions (PRD §4.13)', () => { + it('CONFLICT (github-specific) when no github adapter is configured, even with a linear one (GHSY-03)', async () => { + const { project_id, track_id } = await makeProjectTrack({ withGithub: false }); await upsertAdapter(pool, { projectId: project_id, type: 'linear', @@ -52,50 +116,248 @@ describe('kt_sync_to_github (T2.13 stub)', () => { ); }); - it('negative: 404 when the project does not exist', async () => { - const { track_id } = await makeProjectAndTrack(); - await expect( - syncToGithubService(pool, config, { project_id: UNKNOWN_UUID, track_id }), - ).rejects.toMatchObject({ code: 'NOT_FOUND' }); + it('CONFLICT when the github adapter has no repository configured', async () => { + const { project_id, track_id } = await makeProjectTrack({ repo: undefined }); + await expect(syncToGithubService(pool, config, { project_id, track_id })).rejects.toMatchObject( + { + code: 'CONFLICT', + message: 'github adapter has no repository configured', + }, + ); }); - it('negative: 404 when the track does not exist in the project', async () => { - const { project_id } = await makeProjectAndTrack(); + it('404 when the project does not exist', async () => { + const { track_id } = await makeProjectTrack({ repo: REPO }); await expect( - syncToGithubService(pool, config, { project_id, track_id: UNKNOWN_UUID }), + syncToGithubService(pool, config, { project_id: UNKNOWN_UUID, track_id }), ).rejects.toMatchObject({ code: 'NOT_FOUND' }); }); - it('negative: 404 when the track belongs to a different project (no cross-project leak)', async () => { - const other = await makeProjectAndTrack(); - const { project_id } = await makeProjectAndTrack(); + it('404 when the track belongs to another project (no cross-project leak)', async () => { + const other = await makeProjectTrack({ repo: REPO }); + const { project_id } = await makeProjectTrack({ repo: REPO }); await expect( syncToGithubService(pool, config, { project_id, track_id: other.track_id }), ).rejects.toMatchObject({ code: 'NOT_FOUND' }); }); +}); - // Reachable through the public kt_register_project path (it provisions - // the adapter from inline credentials), not just fixtures/manual SQL; - // the stub must not fall through to a false success or claim "not - // configured". - it('edge: INTERNAL_ERROR "not available in this build" when a github adapter is configured via kt_register_project', async () => { - const { project_id } = await registerProjectService(pool, config, { - name: 'P', - source_type: 'local', - source_ref: `/tmp/${crypto.randomUUID()}`, - adapters: { github: { personal_access_token: 'ghp_test_secret' } }, +describe('kt_sync_to_github — create / update / no-op', () => { + it('creates an issue, links it, and surfaces the URL on kt_get_track', async () => { + const { project_id, track_id } = await makeProjectTrack({ repo: REPO }); + const { state, deps } = makeFake(); + + const res = await syncToGithubService(pool, config, { project_id, track_id }, deps); + expect(res).toEqual({ ok: true }); + expect(state.createCalls).toHaveLength(1); + + const row = await linkRow(track_id); + expect(row?.sync_state).toBe('linked'); + expect(row?.external_id).toBe('1'); + + const track = await getTrackService(pool, config, { project_id, track_id }); + expect(track.track.github_issue_url).toBe(`https://github.com/${REPO}/issues/1`); + + // a successful sync stamps the watermark (T5.2; SYNC_DRIFT input) + const wm = await pool.query(`SELECT last_github_sync_at FROM tracks WHERE id = $1`, [track_id]); + expect(wm.rows[0].last_github_sync_at).not.toBeNull(); + }); + + it('is a no-op on an unchanged re-sync (no second HTTP call)', async () => { + const { project_id, track_id } = await makeProjectTrack({ repo: REPO }); + const { state, deps } = makeFake(); + await syncToGithubService(pool, config, { project_id, track_id }, deps); + const res = await syncToGithubService(pool, config, { project_id, track_id }, deps); + expect(res).toEqual({ ok: true }); + expect(state.createCalls).toHaveLength(1); + expect(state.updateCalls).toHaveLength(0); + }); + + it('updates the existing issue when the track content changes', async () => { + const { project_id, track_id } = await makeProjectTrack({ repo: REPO }); + const { state, deps } = makeFake(); + await syncToGithubService(pool, config, { project_id, track_id }, deps); + // change content -> hash differs -> PATCH + await createItemService(pool, config, { + project_id, + track_id, + title: 'New item', + sequence_position: undefined, + depends_on: [], }); - const { track_id } = await createTrackService(pool, config, { + const res = await syncToGithubService(pool, config, { project_id, track_id }, deps); + expect(res).toEqual({ ok: true }); + expect(state.createCalls).toHaveLength(1); + expect(state.updateCalls).toHaveLength(1); + expect(state.updateCalls[0]!.number).toBe('1'); + }); + + it('closes the issue for a done track (create then close PATCH)', async () => { + const { project_id, track_id } = await makeProjectTrack({ repo: REPO }); + const item = await createItemService(pool, config, { project_id, - title: 'T', + track_id, + title: 'only item', + sequence_position: undefined, depends_on: [], - source_doc_ref: undefined, }); - await expect(syncToGithubService(pool, config, { project_id, track_id })).rejects.toMatchObject( - { - code: 'INTERNAL_ERROR', - message: 'github sync is not available in this build', - }, + await updateItemStatusService(pool, config, { + project_id, + item_id: item.item_id, + status: 'done', + }); + const { state, deps } = makeFake(); + const res = await syncToGithubService(pool, config, { project_id, track_id }, deps); + expect(res).toEqual({ ok: true }); + expect(state.createCalls).toHaveLength(1); + // a done track closes the freshly-created (open) issue via a follow-up PATCH + expect(state.updateCalls).toHaveLength(1); + expect(state.updateCalls[0]!.payload.state).toBe('closed'); + }); +}); + +describe('kt_sync_to_github — operational failures ({ok:false}, never throw)', () => { + it('passes through a definitive failure and clears the pending claim', async () => { + const { project_id, track_id } = await makeProjectTrack({ repo: REPO }); + const { deps } = makeFake({ + createIssue: () => ({ ok: false, error: 'GITHUB_RATE_LIMITED: slow down', ambiguous: false }), + }); + const res = await syncToGithubService(pool, config, { project_id, track_id }, deps); + expect(res.ok).toBe(false); + expect(String(res.error)).toMatch(/^GITHUB_RATE_LIMITED/); + // definitive failure -> no issue created -> pending row cleared + expect(await linkRow(track_id)).toBeUndefined(); + }); + + it('keeps the pending claim after an ambiguous (timeout) failure', async () => { + const { project_id, track_id } = await makeProjectTrack({ repo: REPO }); + const { deps } = makeFake({ + createIssue: () => ({ ok: false, error: 'GITHUB_TIMEOUT: too slow', ambiguous: true }), + }); + const res = await syncToGithubService(pool, config, { project_id, track_id }, deps); + expect(res.ok).toBe(false); + expect(String(res.error)).toMatch(/^GITHUB_TIMEOUT/); + const row = await linkRow(track_id); + expect(row?.sync_state).toBe('pending'); + }); + + it('returns {ok:false, GITHUB_NOT_FOUND} on a 404 update and does NOT recreate', async () => { + const { project_id, track_id } = await makeProjectTrack({ repo: REPO }); + const first = makeFake(); + await syncToGithubService(pool, config, { project_id, track_id }, first.deps); + // change content so an update is attempted + await createItemService(pool, config, { + project_id, + track_id, + title: 'x', + sequence_position: undefined, + depends_on: [], + }); + const { state, deps } = makeFake({ + updateIssue: () => ({ ok: false, error: 'GITHUB_NOT_FOUND: gone', ambiguous: false }), + }); + const res = await syncToGithubService(pool, config, { project_id, track_id }, deps); + expect(res.ok).toBe(false); + expect(String(res.error)).toMatch(/^GITHUB_NOT_FOUND/); + expect(state.createCalls).toHaveLength(0); // no auto-recreate + expect((await linkRow(track_id))?.sync_state).toBe('linked'); // link unchanged + }); + + it('refuses to update when the adapter repo changed under an existing link', async () => { + const { project_id, track_id } = await makeProjectTrack({ repo: REPO }); + const first = makeFake(); + await syncToGithubService(pool, config, { project_id, track_id }, first.deps); + // repoint the adapter at a different repo (keep a decryptable credential — + // the handler decrypts before it reaches the repo-changed refusal) + await upsertAdapter(pool, { + projectId: project_id, + type: 'github', + encryptedCredential: encryptCredential('ghp_test_secret', config.encryptionKey), + config: { repo: 'SathiaAI/Other' }, + }); + const { state, deps } = makeFake(); + const res = await syncToGithubService(pool, config, { project_id, track_id }, deps); + expect(res.ok).toBe(false); + expect(String(res.error)).toMatch(/refusing to update a different repository/); + expect(state.updateCalls).toHaveLength(0); + expect(state.createCalls).toHaveLength(0); + }); +}); + +describe('kt_sync_to_github — durable creation-intent / recovery', () => { + it('concurrent syncs create exactly one issue (no duplicate)', async () => { + const { project_id, track_id } = await makeProjectTrack({ repo: REPO }); + const { state, deps } = makeFake(); + const [a, b] = await Promise.all([ + syncToGithubService(pool, config, { project_id, track_id }, deps), + syncToGithubService(pool, config, { project_id, track_id }, deps), + ]); + expect(state.createCalls).toHaveLength(1); + const rows = await pool.query( + `SELECT count(*)::int AS n FROM track_external_links WHERE track_id = $1 AND adapter_type='github'`, + [track_id], + ); + expect(rows.rows[0].n).toBe(1); + // at least one call succeeded; neither threw + expect([a.ok, b.ok]).toContain(true); + }); + + it('recovers a crashed pending link by adopting the issue the marker finds', async () => { + const { project_id, track_id } = await makeProjectTrack({ repo: REPO }); + // simulate a crash after the pending claim was committed but before finalize + await pool.query( + `INSERT INTO track_external_links (track_id, adapter_type, sync_state, repo_identity, operation_id) + VALUES ($1, 'github', 'pending', $2, $3)`, + [track_id, REPO, randomUUID()], + ); + const { state, deps } = makeFake({ + findIssueByMarker: () => ({ + ok: true, + value: { number: '42', html_url: `https://github.com/${REPO}/issues/42` }, + }), + }); + const res = await syncToGithubService(pool, config, { project_id, track_id }, deps); + expect(res).toEqual({ ok: true }); + expect(state.findCalls).toHaveLength(1); + expect(state.createCalls).toHaveLength(0); // adopted, not recreated + const row = await linkRow(track_id); + expect(row?.sync_state).toBe('linked'); + expect(row?.external_id).toBe('42'); + }); + + it('never recreates on a marker miss — surfaces the pending link for resolution (no duplicate)', async () => { + const { project_id, track_id } = await makeProjectTrack({ repo: REPO }); + // even an old pending row is not recreated: a Search miss cannot prove the + // earlier (ambiguous) create did not land, so we refuse rather than risk a + // duplicate (T5.2: duplicate prevention over auto-retry). + await pool.query( + `INSERT INTO track_external_links (track_id, adapter_type, sync_state, repo_identity, operation_id, created_at) + VALUES ($1, 'github', 'pending', $2, $3, now() - interval '2 hours')`, + [track_id, REPO, randomUUID()], + ); + const { state, deps } = makeFake(); // findIssueByMarker default -> null + const res = await syncToGithubService(pool, config, { project_id, track_id }, deps); + expect(res.ok).toBe(false); + expect(String(res.error)).toMatch(/not auto-recreated/); + expect(state.findCalls).toHaveLength(1); + expect(state.createCalls).toHaveLength(0); // NEVER recreate on a miss + expect((await linkRow(track_id))?.sync_state).toBe('pending'); + }); + + it('refuses recovery when the adapter repo changed while a sync was pending (no wrong-repo search/recreate)', async () => { + const { project_id, track_id } = await makeProjectTrack({ repo: REPO }); + // pending row bound to a DIFFERENT repo than the adapter now points at + await pool.query( + `INSERT INTO track_external_links (track_id, adapter_type, sync_state, repo_identity, operation_id, created_at) + VALUES ($1, 'github', 'pending', 'SathiaAI/OldRepo', $2, now() - interval '5 minutes')`, + [track_id, randomUUID()], ); + const { state, deps } = makeFake(); + const res = await syncToGithubService(pool, config, { project_id, track_id }, deps); + expect(res.ok).toBe(false); + expect(String(res.error)).toMatch(/resolve the repository change/); + expect(state.findCalls).toHaveLength(0); // never searched the wrong repo + expect(state.createCalls).toHaveLength(0); + expect((await linkRow(track_id))?.sync_state).toBe('pending'); }); }); diff --git a/tests/unit/github-client.test.ts b/tests/unit/github-client.test.ts new file mode 100644 index 0000000..17cd28d --- /dev/null +++ b/tests/unit/github-client.test.ts @@ -0,0 +1,252 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { createFetchGitHubClient } from '../../src/github/github-client.js'; + +const TOKEN = 'ghp_super_secret_value'; +const OPTS = { timeoutMs: 8000, userAgent: 'knotrack-mcp-server' }; + +function client() { + return createFetchGitHubClient(TOKEN, OPTS); +} + +interface FakeResponse { + ok: boolean; + status: number; + headers: Headers; + text: () => Promise; +} + +function resp(status: number, body: string, headers: Record = {}): FakeResponse { + return { + ok: status >= 200 && status < 300, + status, + headers: new Headers(headers), + text: () => Promise.resolve(body), + }; +} + +interface FetchInit { + method: string; + headers: Record; + body: string; +} + +afterEach(() => { + vi.unstubAllGlobals(); +}); + +describe('createFetchGitHubClient — request shape', () => { + it('createIssue POSTs title+body with the correct headers and URL', async () => { + const fetchMock = vi.fn((_url: string, _init: FetchInit) => + Promise.resolve( + resp(201, JSON.stringify({ number: 7, html_url: 'https://github.com/o/r/issues/7' })), + ), + ); + vi.stubGlobal('fetch', fetchMock); + + const res = await client().createIssue('o/r', { title: 'T', body: 'B', state: 'open' }); + expect(res).toEqual({ + ok: true, + value: { number: '7', html_url: 'https://github.com/o/r/issues/7' }, + }); + + const [url, init] = fetchMock.mock.calls[0]!; + expect(url).toBe('https://api.github.com/repos/o/r/issues'); + expect(init.method).toBe('POST'); + expect(init.headers.Authorization).toBe(`Bearer ${TOKEN}`); + expect(init.headers.Accept).toBe('application/vnd.github+json'); + expect(init.headers['X-GitHub-Api-Version']).toBe('2022-11-28'); + expect(init.headers['User-Agent']).toBe('knotrack-mcp-server'); + expect(JSON.parse(init.body)).toEqual({ title: 'T', body: 'B' }); + }); + + it('updateIssue PATCHes title+body+state(+state_reason) at the numbered URL', async () => { + const fetchMock = vi.fn((_url: string, _init: FetchInit) => + Promise.resolve( + resp(200, JSON.stringify({ number: 7, html_url: 'https://github.com/o/r/issues/7' })), + ), + ); + vi.stubGlobal('fetch', fetchMock); + + await client().updateIssue('o/r', '7', { + title: 'T', + body: 'B', + state: 'closed', + state_reason: 'completed', + }); + const [url, init] = fetchMock.mock.calls[0]!; + expect(url).toBe('https://api.github.com/repos/o/r/issues/7'); + expect(init.method).toBe('PATCH'); + expect(JSON.parse(init.body)).toEqual({ + title: 'T', + body: 'B', + state: 'closed', + state_reason: 'completed', + }); + }); + + it('findIssueByMarker searches and returns only a body that actually contains the marker', async () => { + const marker = ''; + const fetchMock = vi.fn((_url: string) => + Promise.resolve( + resp( + 200, + JSON.stringify({ + items: [ + { number: 1, html_url: 'u1', body: 'unrelated' }, + { number: 2, html_url: 'u2', body: `has ${marker} inside` }, + ], + }), + ), + ), + ); + vi.stubGlobal('fetch', fetchMock); + + const res = await client().findIssueByMarker('o/r', marker); + expect(res).toEqual({ ok: true, value: { number: '2', html_url: 'u2' } }); + const url = fetchMock.mock.calls[0]![0]; + expect(url).toContain('/search/issues?q='); + // excludes pull requests + expect(decodeURIComponent(url)).toContain('type:issue'); + }); + + it('findIssueByMarker returns null when a COMPLETE search finds no match', async () => { + vi.stubGlobal( + 'fetch', + vi.fn(() => + Promise.resolve( + resp( + 200, + JSON.stringify({ + total_count: 1, + incomplete_results: false, + items: [{ number: 1, html_url: 'u', body: 'nope' }], + }), + ), + ), + ), + ); + const res = await client().findIssueByMarker('o/r', ''); + expect(res).toEqual({ ok: true, value: null }); + }); + + it('findIssueByMarker reports unconfirmed (not null) when the search is incomplete', async () => { + vi.stubGlobal( + 'fetch', + vi.fn(() => + Promise.resolve( + resp(200, JSON.stringify({ total_count: 0, incomplete_results: true, items: [] })), + ), + ), + ); + const res = await client().findIssueByMarker('o/r', ''); + expect(res.ok).toBe(false); + if (!res.ok) expect(res.ambiguous).toBe(true); + }); + + it('findIssueByMarker reports unconfirmed when more matches exist than were fetched', async () => { + vi.stubGlobal( + 'fetch', + vi.fn(() => + Promise.resolve( + resp( + 200, + JSON.stringify({ + total_count: 50, + incomplete_results: false, + items: [{ number: 1, html_url: 'u', body: 'nope' }], + }), + ), + ), + ), + ); + const res = await client().findIssueByMarker('o/r', ''); + expect(res.ok).toBe(false); + }); +}); + +describe('createFetchGitHubClient — error mapping (PRD §4.13 prefixes)', () => { + const cases: Array<[number, Record, string, boolean]> = [ + // [status, headers, prefix, ambiguous] + [401, {}, 'GITHUB_AUTH_FAILED', false], + [403, { 'x-ratelimit-remaining': '0' }, 'GITHUB_RATE_LIMITED', false], + [403, { 'retry-after': '60' }, 'GITHUB_RATE_LIMITED', false], + [403, {}, 'GITHUB_AUTH_FAILED', false], + [404, {}, 'GITHUB_NOT_FOUND', false], + [422, {}, 'GITHUB_UNKNOWN_ERROR', false], + [429, {}, 'GITHUB_RATE_LIMITED', false], + // 5xx is ambiguous: the create may have landed server-side. + [500, {}, 'GITHUB_UNKNOWN_ERROR', true], + [503, {}, 'GITHUB_UNKNOWN_ERROR', true], + ]; + + for (const [status, headers, prefix, ambiguous] of cases) { + it(`maps HTTP ${status} -> ${prefix} (ambiguous=${ambiguous}, no token leak)`, async () => { + vi.stubGlobal( + 'fetch', + vi.fn(() => + Promise.resolve(resp(status, JSON.stringify({ message: 'gh says no' }), headers)), + ), + ); + const res = await client().createIssue('o/r', { title: 'T', body: 'B', state: 'open' }); + expect(res.ok).toBe(false); + if (!res.ok) { + expect(res.error.startsWith(prefix)).toBe(true); + expect(res.ambiguous).toBe(ambiguous); + expect(res.error).not.toContain(TOKEN); + } + }); + } + + it('maps a 403 secondary-rate-limit body -> GITHUB_RATE_LIMITED (not auth failure)', async () => { + vi.stubGlobal( + 'fetch', + vi.fn(() => + Promise.resolve( + resp(403, JSON.stringify({ message: 'You have exceeded a secondary rate limit' }), {}), + ), + ), + ); + const res = await client().createIssue('o/r', { title: 'T', body: 'B', state: 'open' }); + expect(res.ok).toBe(false); + if (!res.ok) expect(res.error.startsWith('GITHUB_RATE_LIMITED')).toBe(true); + }); + + it('maps an AbortError (timeout) -> GITHUB_TIMEOUT with ambiguous=true', async () => { + vi.stubGlobal( + 'fetch', + vi.fn(() => Promise.reject(Object.assign(new Error('aborted'), { name: 'AbortError' }))), + ); + const res = await client().createIssue('o/r', { title: 'T', body: 'B', state: 'open' }); + expect(res.ok).toBe(false); + if (!res.ok) { + expect(res.error.startsWith('GITHUB_TIMEOUT')).toBe(true); + expect(res.ambiguous).toBe(true); + } + }); + + it('treats a 2xx create with malformed JSON as ambiguous (issue may exist)', async () => { + vi.stubGlobal( + 'fetch', + vi.fn(() => Promise.resolve(resp(201, 'not json <<<'))), + ); + const res = await client().createIssue('o/r', { title: 'T', body: 'B', state: 'open' }); + expect(res.ok).toBe(false); + if (!res.ok) { + expect(res.error.startsWith('GITHUB_UNKNOWN_ERROR')).toBe(true); + expect(res.ambiguous).toBe(true); + } + }); + + it('maps a network throw -> GITHUB_UNKNOWN_ERROR with ambiguous=true', async () => { + vi.stubGlobal( + 'fetch', + vi.fn(() => Promise.reject(new Error('ECONNRESET'))), + ); + const res = await client().createIssue('o/r', { title: 'T', body: 'B', state: 'open' }); + expect(res.ok).toBe(false); + if (!res.ok) { + expect(res.error.startsWith('GITHUB_UNKNOWN_ERROR')).toBe(true); + expect(res.ambiguous).toBe(true); + } + }); +}); diff --git a/tests/unit/issue-payload.test.ts b/tests/unit/issue-payload.test.ts new file mode 100644 index 0000000..b4d5c2c --- /dev/null +++ b/tests/unit/issue-payload.test.ts @@ -0,0 +1,174 @@ +import { describe, expect, it } from 'vitest'; +import { + buildIssuePayload, + payloadContentHash, + trackMarker, + type ItemForIssue, +} from '../../src/mcp/tools/issue-payload.js'; + +const TRACK_ID = '11111111-1111-4111-8111-111111111111'; + +function items(...specs: Array<[string, string, number]>): ItemForIssue[] { + return specs.map(([title, status, sequence_position], i) => ({ + id: `item-${i}`, + title, + status, + sequence_position, + })); +} + +describe('buildIssuePayload', () => { + it('maps title, open state, checklist in sequence order, and embeds the hidden marker', () => { + const payload = buildIssuePayload( + { id: TRACK_ID, title: 'Auth overhaul', status: 'on_track' }, + items(['Rotation tests', 'pending', 2], ['Refresh endpoint', 'done', 1]), + ); + expect(payload.title).toBe('Auth overhaul'); + expect(payload.state).toBe('open'); + expect(payload.state_reason).toBeUndefined(); + // sequence order, done -> [x], others -> [ ] + const refreshIdx = payload.body.indexOf('- [x] Refresh endpoint'); + const rotationIdx = payload.body.indexOf('- [ ] Rotation tests'); + expect(refreshIdx).toBeGreaterThanOrEqual(0); + expect(rotationIdx).toBeGreaterThan(refreshIdx); + expect(payload.body).toContain(trackMarker(TRACK_ID)); + expect(payload.body).toContain('`on_track`'); + }); + + it('closes the issue only for a done track, with state_reason completed', () => { + const done = buildIssuePayload({ id: TRACK_ID, title: 'T', status: 'done' }, []); + expect(done.state).toBe('closed'); + expect(done.state_reason).toBe('completed'); + + for (const status of ['on_track', 'pivot_pending', 'blocked']) { + const open = buildIssuePayload({ id: TRACK_ID, title: 'T', status }, []); + expect(open.state).toBe('open'); + expect(open.state_reason).toBeUndefined(); + } + }); + + it('renders a placeholder when the track has no items', () => { + const payload = buildIssuePayload({ id: TRACK_ID, title: 'T', status: 'on_track' }, []); + expect(payload.body).toContain('_No items yet._'); + }); + + it('truncates an over-long title to 256 chars', () => { + const long = 'x'.repeat(500); + const payload = buildIssuePayload({ id: TRACK_ID, title: long, status: 'on_track' }, []); + expect(payload.title).toHaveLength(256); + }); + + it('keeps the title within 256 chars even when escaping delimiters expands it', () => { + // Each `` to `-->` (+3). + // A near-limit title full of these must be truncated AFTER escaping, or it + // would exceed GitHub's 256-char cap and 422 the sync. + const dense = ''.repeat(80); // 560 raw chars, all delimiter bytes + const payload = buildIssuePayload({ id: TRACK_ID, title: dense, status: 'on_track' }, []); + expect(payload.title.length).toBeLessThanOrEqual(256); + // And the truncated title must not contain a raw, unescaped marker opener. + expect(payload.title).not.toContain('