From cb4ab0c09b78f8f65a0b77ad4798dd58aba5e9a3 Mon Sep 17 00:00:00 2001 From: Paul Poulose Date: Sat, 19 Sep 2026 09:03:16 -0400 Subject: [PATCH 01/14] docs(roadmap): mark T5.1 credential-encryption verified done; unblock T5 T4 (done 2026-09-07) and T2.16 (shipped 2026-09-08) are both cleared, so T5's stale 'blocked' no longer holds -> on_track. T5.1's at-rest encryption acceptance is confirmed met by src/crypto/credential-cipher.ts plus the unit and register-project integration tests (decrypt round-trip read back from the adapters table). No code change; verification only. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01278w3fed8ZcXjHpKrFHWDw --- docs/ROADMAP.md | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index 3f93fc5..f63a090 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -949,17 +949,35 @@ 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** (see that item); `T5.2`–`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`. + **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` 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 From 91e4d8d6a4f708f0fbb58c6aa2ae26bea09b3504 Mon Sep 17 00:00:00 2001 From: Paul Poulose Date: Sat, 19 Sep 2026 09:31:22 -0400 Subject: [PATCH 02/14] feat(t5.2): implement kt_sync_to_github (track->Issue, hardened idempotency) Real GitHub Issue sync for a track (PRD 4.13; ROADMAP T5.2), replacing the T2.13 stub. Design panel 2026-09-19 chose Option A (persisted link + native fetch, no SDK), unanimous; Paul chose the fully-hardened durable creation-intent variant. - migrations/009_track_external_links: authoritative (track, adapter_type) link, UNIQUE(track_id, adapter_type), sync_state pending|linked, external_id=issue number, repo_identity binding, content_hash, operation_id. - src/github/github-client.ts: thin native-fetch client (create/update/ findByMarker), AbortController timeout, PRD error prefixes (GITHUB_AUTH_FAILED/NOT_FOUND/RATE_LIMITED/TIMEOUT/UNKNOWN_ERROR), ambiguity flag; never logs the token. - issue-payload.ts: pure track->issue mapping (title<=256, body with hidden knotrack:track marker, <=65536, closed iff track done) + sha256 no-op hash. - track-external-links queries + getAdapterForProject single-row read. - sync-to-github handler: pending claim committed before POST; noop/update/ create/pending-recovery state machine; missing/unusable adapter -> CONFLICT; operational GitHub failures -> {ok:false, error} (never throw); 404-on-PATCH is not auto-recreated. - get-track: surface github_issue_url on the track read contract (join, no redundant tracks column). Tests + doc reconciliation follow in subsequent commits. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01278w3fed8ZcXjHpKrFHWDw --- migrations/009_track_external_links.down.sql | 4 + migrations/009_track_external_links.sql | 56 ++++ src/db/queries/adapters.ts | 19 ++ src/db/queries/track-external-links.ts | 161 +++++++++ src/github/github-client.ts | 216 +++++++++++++ src/mcp/tools/get-track.ts | 7 + src/mcp/tools/issue-payload.ts | 109 +++++++ src/mcp/tools/sync-to-github.ts | 323 ++++++++++++++++++- 8 files changed, 889 insertions(+), 6 deletions(-) create mode 100644 migrations/009_track_external_links.down.sql create mode 100644 migrations/009_track_external_links.sql create mode 100644 src/db/queries/track-external-links.ts create mode 100644 src/github/github-client.ts create mode 100644 src/mcp/tools/issue-payload.ts diff --git a/migrations/009_track_external_links.down.sql b/migrations/009_track_external_links.down.sql new file mode 100644 index 0000000..89e98ba --- /dev/null +++ b/migrations/009_track_external_links.down.sql @@ -0,0 +1,4 @@ +-- Down: drop track_external_links (T5.2). The shared set_updated_at() +-- function is left in place — it is used by other tables. Dropping the +-- table removes its own trigger and index automatically. +DROP TABLE IF EXISTS track_external_links; diff --git a/migrations/009_track_external_links.sql b/migrations/009_track_external_links.sql new file mode 100644 index 0000000..4ce638b --- /dev/null +++ b/migrations/009_track_external_links.sql @@ -0,0 +1,56 @@ +-- ============================================================ +-- 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. + +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(); 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/github/github-client.ts b/src/github/github-client.ts new file mode 100644 index 0000000..e95a2bc --- /dev/null +++ b/src/github/github-client.ts @@ -0,0 +1,216 @@ +// 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): 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; + } + return false; +} + +/** Maps a completed HTTP response (non-2xx) to a prefixed error string plus + * the ambiguity flag. A response means the request reached GitHub, so a + * create that got any response did NOT create an issue (GitHub rejected + * it): ambiguous=false for every mapped status here. Only never-completed + * requests (timeout/network) are ambiguous, handled 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)) + 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 client/server status fold into UNKNOWN with GitHub's own message. + return { error: `GITHUB_UNKNOWN_ERROR: HTTP ${status}${detail ? `: ${detail}` : ''}`, ambiguous: false }; +} + +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', + ambiguous: false, + }; + } + 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. GitHub search is eventually consistent, so the caller + // must treat a null result together with the pending link's age, never + // as immediate proof no issue exists. + const q = encodeURIComponent(`repo:${repo} in:body "${marker}"`); + const res = await send('GET', `${GITHUB_API}/search/issues?q=${q}&per_page=10`); + if (!res.ok) return res; + const body = res.value.body; + const items = + body && typeof body === 'object' && Array.isArray((body as Record).items) + ? ((body as Record).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 }; + } + } + } + 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..b6ba853 --- /dev/null +++ b/src/mcp/tools/issue-payload.ts @@ -0,0 +1,109 @@ +// 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 { + 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]' : '[ ]'; +} + +/** 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 { + const title = truncate(track.title, TITLE_MAX); + const isDone = track.status === 'done'; + + const ordered = [...items].sort((a, b) => a.sequence_position - b.sequence_position); + const shown = ordered.slice(0, ITEM_RENDER_CAP); + const checklist = shown.map((i) => `- ${itemCheckbox(i.status)} ${i.title}`).join('\n'); + const elided = + ordered.length > shown.length + ? `\n\n_… ${ordered.length - shown.length} more item(s) not shown._` + : ''; + + const bodyFull = [ + '_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', + checklist.length > 0 ? checklist : '_No items yet._', + elided, + '', + trackMarker(track.id), + ].join('\n'); + + const payload: IssuePayload = { + title, + body: truncate(bodyFull, BODY_MAX), + 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..2d7577b 100644 --- a/src/mcp/tools/sync-to-github.ts +++ b/src/mcp/tools/sync-to-github.ts @@ -1,17 +1,328 @@ -// 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 } 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 { 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'; +// Below this age a `pending` link whose issue the marker search can't find +// might just be un-indexed (GitHub search is eventually consistent), so we +// ask the caller to retry rather than recreate. Past it, a search miss is +// strong enough evidence to clear and recreate. +const RECOVERY_INDEX_LAG_MS = 60_000; + +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. + const items = await listItemsByTrack(pool, track.id); + const payload = buildIssuePayload( + { id: track.id, title: track.title, status: track.status }, + items.map((i) => ({ 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': + return { ok: true }; + 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); + case 'create': + return createFresh(pool, client, repo, track.id, decision.operationId, payload, hash); + case 'pending': + return recoverPending(pool, client, repo, track.id, decision.row, payload, hash); + } +} + +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, +): 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 { ok: true }; +} + +async function updateExisting( + pool: Pool, + client: GitHubClient, + repo: string, + trackId: string, + row: TrackExternalLinkRow, + payload: GitHubIssuePayload, + hash: string, +): 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', + }; + } + 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 { ok: true }; +} + +async function recoverPending( + pool: Pool, + client: GitHubClient, + repo: string, + trackId: string, + row: TrackExternalLinkRow, + payload: GitHubIssuePayload, + hash: string, +): Promise { + 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 { ok: true }; + } + + // Marker not found. Search is eventually consistent, so only treat a miss + // as authoritative once the pending row is older than the index-lag window. + const ageMs = Date.now() - row.created_at.getTime(); + if (ageMs <= RECOVERY_INDEX_LAG_MS) { + return { + ok: false, + error: + 'GITHUB_UNKNOWN_ERROR: a previous sync did not complete and its outcome is not yet confirmable; retry shortly', + }; + } + const operationId = randomUUID(); + const reclaimed = await withTransaction(pool, async (c) => { + const cur = await getLinkForUpdate(c, trackId, ADAPTER); + if (!cur || cur.sync_state !== 'pending' || cur.operation_id !== row.operation_id) { + return false; + } + await clearPendingLink(c, { trackId, adapterType: ADAPTER, operationId: row.operation_id }); + const claimed = await claimPendingLink(c, { + trackId, + adapterType: ADAPTER, + repoIdentity: repo, + operationId, + }); + return claimed !== undefined; + }); + if (!reclaimed) { + return { + ok: false, + error: 'GITHUB_UNKNOWN_ERROR: pending sync changed during recovery; retry shortly', + }; + } + return createFresh(pool, client, repo, trackId, operationId, payload, hash); } export function registerSyncToGithubTool( @@ -25,7 +336,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) => { From b3ef499e413edce513b04601be1280fbd177bfe9 Mon Sep 17 00:00:00 2001 From: Paul Poulose Date: Sat, 19 Sep 2026 09:40:22 -0400 Subject: [PATCH 03/14] test(t5.2): unit + integration matrix for kt_sync_to_github MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Unit (no DB): issue-payload mapping (title/body/marker/checklist/state, truncation) + content-hash stability/sensitivity; github-client request shape (headers/url/method/body) + PRD error-prefix mapping per HTTP status, timeout ambiguity, and no-token-leak. Integration (real DB, injected fake GitHubClient — no network): preconditions (CONFLICT no-adapter / no-repo, 404s incl. cross-project), create+link+URL on kt_get_track, no-op-by-hash, update-on-change, done-track close, definitive vs ambiguous failure handling (clear vs keep pending), 404-on-update no-recreate, repo-changed refusal, concurrent-syncs-one-issue, and pending-link recovery (adopt via marker / aged-miss recreate / fresh-miss retry). Replaces the old T2.13 stub test. 23 unit tests pass locally; integration suite runs against CI Postgres. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01278w3fed8ZcXjHpKrFHWDw --- tests/integration/sync-to-github.test.ts | 330 ++++++++++++++++++++--- tests/unit/github-client.test.ts | 178 ++++++++++++ tests/unit/issue-payload.test.ts | 118 ++++++++ 3 files changed, 586 insertions(+), 40 deletions(-) create mode 100644 tests/unit/github-client.test.ts create mode 100644 tests/unit/issue-payload.test.ts diff --git a/tests/integration/sync-to-github.test.ts b/tests/integration/sync-to-github.test.ts index 5539637..542bca4 100644 --- a/tests/integration/sync-to-github.test.ts +++ b/tests/integration/sync-to-github.test.ts @@ -1,42 +1,105 @@ +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 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 +115,237 @@ 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' }); }); +}); + +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`); + }); + + 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); + }); - // 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' } }, + 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 + await upsertAdapter(pool, { + projectId: project_id, + type: 'github', + encryptedCredential: Buffer.from('x'), + 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('recreates when an aged pending link has no findable issue', async () => { + const { project_id, track_id } = await makeProjectTrack({ repo: REPO }); + 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 minutes')`, + [track_id, REPO, randomUUID()], + ); + const { state, deps } = makeFake(); // findIssueByMarker default -> null + 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(1); // aged miss -> safe to recreate + expect((await linkRow(track_id))?.sync_state).toBe('linked'); + }); + + it('asks the caller to retry (no recreate) for a fresh pending link with no findable issue', async () => { + const { project_id, track_id } = await makeProjectTrack({ repo: REPO }); + 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(); + const res = await syncToGithubService(pool, config, { project_id, track_id }, deps); + expect(res.ok).toBe(false); + expect(String(res.error)).toMatch(/retry shortly/); + 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..d7efed2 --- /dev/null +++ b/tests/unit/github-client.test.ts @@ -0,0 +1,178 @@ +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' } }); + expect(fetchMock.mock.calls[0]![0]).toContain('/search/issues?q='); + }); + + it('findIssueByMarker returns null when no body matches', async () => { + vi.stubGlobal( + 'fetch', + vi.fn(() => + Promise.resolve( + resp(200, JSON.stringify({ items: [{ number: 1, html_url: 'u', body: 'nope' }] })), + ), + ), + ); + const res = await client().findIssueByMarker('o/r', ''); + expect(res).toEqual({ ok: true, value: null }); + }); +}); + +describe('createFetchGitHubClient — error mapping (PRD §4.13 prefixes)', () => { + const cases: Array<[number, Record, string]> = [ + [401, {}, 'GITHUB_AUTH_FAILED'], + [403, { 'x-ratelimit-remaining': '0' }, 'GITHUB_RATE_LIMITED'], + [403, { 'retry-after': '60' }, 'GITHUB_RATE_LIMITED'], + [403, {}, 'GITHUB_AUTH_FAILED'], + [404, {}, 'GITHUB_NOT_FOUND'], + [422, {}, 'GITHUB_UNKNOWN_ERROR'], + [429, {}, 'GITHUB_RATE_LIMITED'], + [500, {}, 'GITHUB_UNKNOWN_ERROR'], + ]; + + for (const [status, headers, prefix] of cases) { + it(`maps HTTP ${status} -> ${prefix} (ambiguous=false, 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(false); + expect(res.error).not.toContain(TOKEN); + } + }); + } + + 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('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..44632f6 --- /dev/null +++ b/tests/unit/issue-payload.test.ts @@ -0,0 +1,118 @@ +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]) => ({ 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('truncates the body to GitHub’s 65,536-char cap', () => { + const many = items( + ...Array.from( + { length: 400 }, + (_, i) => ['y'.repeat(300), 'pending', i] as [string, string, number], + ), + ); + const payload = buildIssuePayload({ id: TRACK_ID, title: 'T', status: 'on_track' }, many); + expect(payload.body.length).toBeLessThanOrEqual(65_536); + }); + + it('caps the rendered checklist and notes the elision', () => { + const many = items( + ...Array.from( + { length: 350 }, + (_, i) => [`item ${i}`, 'pending', i] as [string, string, number], + ), + ); + const payload = buildIssuePayload({ id: TRACK_ID, title: 'T', status: 'on_track' }, many); + expect(payload.body).toMatch(/more item\(s\) not shown/); + }); +}); + +describe('payloadContentHash', () => { + it('is stable for identical payloads and 64 hex chars', () => { + const p = buildIssuePayload( + { id: TRACK_ID, title: 'T', status: 'on_track' }, + items(['A', 'pending', 1]), + ); + const h1 = payloadContentHash(p); + const h2 = payloadContentHash( + buildIssuePayload( + { id: TRACK_ID, title: 'T', status: 'on_track' }, + items(['A', 'pending', 1]), + ), + ); + expect(h1).toBe(h2); + expect(h1).toMatch(/^[0-9a-f]{64}$/); + }); + + it('changes when an item status changes (so a real change re-syncs)', () => { + const before = payloadContentHash( + buildIssuePayload( + { id: TRACK_ID, title: 'T', status: 'on_track' }, + items(['A', 'pending', 1]), + ), + ); + const after = payloadContentHash( + buildIssuePayload({ id: TRACK_ID, title: 'T', status: 'on_track' }, items(['A', 'done', 1])), + ); + expect(before).not.toBe(after); + }); + + it('changes when the track open/closed state changes', () => { + const open = payloadContentHash( + buildIssuePayload({ id: TRACK_ID, title: 'T', status: 'on_track' }, []), + ); + const closed = payloadContentHash( + buildIssuePayload({ id: TRACK_ID, title: 'T', status: 'done' }, []), + ); + expect(open).not.toBe(closed); + }); +}); From fabd779ae833f66f49f1434f42d36357b33328db Mon Sep 17 00:00:00 2001 From: Paul Poulose Date: Sat, 19 Sep 2026 09:44:55 -0400 Subject: [PATCH 04/14] docs(t5.2): reconcile ROADMAP/PRD/TRD/README/TEST_CASES to the shipped sync MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - ROADMAP T5.2: mark implemented + offline-verified; correct the stale "item" wording to "track" (drift vs the shipped tool schema and PRD §4.13, which is track-scoped with no item-level target); document the durable creation-intent, the deferred real-repo dogfood gate, and that retries/backoff are intentionally absent. - PRD §4.13: describe the implemented behavior (idempotent create/update, URL on kt_get_track.track.github_issue_url), add the no-repo CONFLICT and the no-duplicate/idempotency rules; refresh acceptance criteria. - TRD §3.14: extend the 409 precondition to the no-repo case; add the idempotency/recovery note (the prefixes already matched the impl). - README: kt_sync_to_github stub -> implemented (12/14 implemented). - TEST_CASES: GHSY note updated; add GHSY-14..19 (create+read, no-op, update, no-repo CONFLICT, crash recovery, no silent retarget). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01278w3fed8ZcXjHpKrFHWDw --- README.md | 48 ++++++++++++++++++++++++---------------------- docs/PRD.md | 14 ++++++++------ docs/ROADMAP.md | 37 +++++++++++++++++++++++++++++------ docs/TEST_CASES.md | 8 +++++++- docs/TRD.md | 6 ++++-- 5 files changed, 75 insertions(+), 38 deletions(-) diff --git a/README.md b/README.md index 92ab85e..ee226e5 100644 --- a/README.md +++ b/README.md @@ -16,15 +16,17 @@ harness — not just one vendor's tool. ## Status -Pre-release, v0.1.0. 11 of the 14 planned tools are fully implemented and +Pre-release, v0.1.0. 12 of the 14 planned tools are fully implemented and 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, offline-verified, with its one +remaining gate a real-repo dogfood run. 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 +42,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..dbd895f 100644 --- a/docs/PRD.md +++ b/docs/PRD.md @@ -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` diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index f63a090..f2c2a4e 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -952,7 +952,9 @@ this Track. **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** (see that item); `T5.2`–`T5.4` remain open. +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 @@ -978,11 +980,34 @@ corrected track-status model, not the currently-broken one). "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` 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`. +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). 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..ad9fcde 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 create | Same call | Recovers via the hidden `` marker — adopts the existing Issue if found, recreates only after an aged search-miss, asks the caller to retry on a fresh miss — never creating a duplicate | +| 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` From d662025aff8de940918d5ad22c5182dfc859bcf6 Mon Sep 17 00:00:00 2001 From: Paul Poulose Date: Sat, 19 Sep 2026 09:52:48 -0400 Subject: [PATCH 05/14] style(t5.2): apply prettier to github-client + sync-to-github The pinned prettier reformatting of these two files landed in the working tree after their feat commit but was never staged, so CI format:check flagged the committed (pre-format) blobs. Commit the formatted versions. No behavior change. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01278w3fed8ZcXjHpKrFHWDw --- src/github/github-client.ts | 20 ++++++++++++++------ src/mcp/tools/sync-to-github.ts | 6 +++++- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/src/github/github-client.ts b/src/github/github-client.ts index e95a2bc..2c08ca7 100644 --- a/src/github/github-client.ts +++ b/src/github/github-client.ts @@ -30,8 +30,7 @@ export interface GitHubIssueRef { } export type GitHubResult = - | { ok: true; value: T } - | { ok: false; error: string; ambiguous: boolean }; + { ok: true; value: T } | { ok: false; error: string; ambiguous: boolean }; export interface GitHubClient { /** POST /repos/{repo}/issues — creates an (open) issue with title+body. */ @@ -68,20 +67,29 @@ function isRateLimited(status: number, headers: Headers): boolean { * create that got any response did NOT create an issue (GitHub rejected * it): ambiguous=false for every mapped status here. Only never-completed * requests (timeout/network) are ambiguous, handled by the caller's catch. */ -function mapErrorResponse(status: number, headers: Headers, bodyText: string): { +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 (status === 401) + return { error: `GITHUB_AUTH_FAILED: ${detail || 'unauthorized'}`, ambiguous: false }; if (isRateLimited(status, headers)) 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 }; + 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 client/server status fold into UNKNOWN with GitHub's own message. - return { error: `GITHUB_UNKNOWN_ERROR: HTTP ${status}${detail ? `: ${detail}` : ''}`, ambiguous: false }; + return { + error: `GITHUB_UNKNOWN_ERROR: HTTP ${status}${detail ? `: ${detail}` : ''}`, + ambiguous: false, + }; } function mapThrown(err: unknown): { error: string; ambiguous: boolean } { diff --git a/src/mcp/tools/sync-to-github.ts b/src/mcp/tools/sync-to-github.ts index 2d7577b..4224c08 100644 --- a/src/mcp/tools/sync-to-github.ts +++ b/src/mcp/tools/sync-to-github.ts @@ -112,7 +112,11 @@ export async function syncToGithubService( const items = await listItemsByTrack(pool, track.id); const payload = buildIssuePayload( { id: track.id, title: track.title, status: track.status }, - items.map((i) => ({ title: i.title, status: i.status, sequence_position: i.sequence_position })), + items.map((i) => ({ + title: i.title, + status: i.status, + sequence_position: i.sequence_position, + })), ); const hash = payloadContentHash(payload); From 5e5c0347e750d8c06941408afd6221c7d48b1548 Mon Sep 17 00:00:00 2001 From: Paul Poulose Date: Sat, 19 Sep 2026 09:55:35 -0400 Subject: [PATCH 06/14] fix(t5.2): wrap migration 009 in BEGIN/COMMIT; guard down-migration The migrate runner (scripts/migrate.ts) requires each file start with BEGIN; and end with COMMIT; (it strips them to control the transaction). Add them. The down migration also refuses to drop track_external_links when rows exist, matching the 005 down convention. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01278w3fed8ZcXjHpKrFHWDw --- migrations/009_track_external_links.down.sql | 24 +++++++++++++++++--- migrations/009_track_external_links.sql | 4 ++++ 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/migrations/009_track_external_links.down.sql b/migrations/009_track_external_links.down.sql index 89e98ba..bca171c 100644 --- a/migrations/009_track_external_links.down.sql +++ b/migrations/009_track_external_links.down.sql @@ -1,4 +1,22 @@ --- Down: drop track_external_links (T5.2). The shared set_updated_at() --- function is left in place — it is used by other tables. Dropping the --- table removes its own trigger and index automatically. +-- 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 + 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 $$; + DROP TABLE IF EXISTS track_external_links; + +COMMIT; diff --git a/migrations/009_track_external_links.sql b/migrations/009_track_external_links.sql index 4ce638b..8808de1 100644 --- a/migrations/009_track_external_links.sql +++ b/migrations/009_track_external_links.sql @@ -23,6 +23,8 @@ -- 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, @@ -54,3 +56,5 @@ CREATE TRIGGER trg_track_external_links_set_updated_at BEFORE UPDATE ON track_external_links FOR EACH ROW EXECUTE FUNCTION set_updated_at(); + +COMMIT; From 6ae89ec032fd5ef00f5f4ae52682b73428c0c233 Mon Sep 17 00:00:00 2001 From: Paul Poulose Date: Sat, 19 Sep 2026 09:58:37 -0400 Subject: [PATCH 07/14] test(t5.2): use a decryptable credential in the repo-changed test The repo-repoint step stored Buffer.from('x') as the credential; the handler decrypts before the repo-changed refusal, so decrypt threw INTERNAL_ERROR instead of returning the {ok:false} refusal. Store a real encrypted credential. Code unchanged. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01278w3fed8ZcXjHpKrFHWDw --- tests/integration/sync-to-github.test.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/integration/sync-to-github.test.ts b/tests/integration/sync-to-github.test.ts index 542bca4..134a7aa 100644 --- a/tests/integration/sync-to-github.test.ts +++ b/tests/integration/sync-to-github.test.ts @@ -7,6 +7,7 @@ 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'; @@ -262,11 +263,12 @@ describe('kt_sync_to_github — operational failures ({ok:false}, never throw)', 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 + // 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: Buffer.from('x'), + encryptedCredential: encryptCredential('ghp_test_secret', config.encryptionKey), config: { repo: 'SathiaAI/Other' }, }); const { state, deps } = makeFake(); From 90bb621924bf92dd0cbcb88f24b0b8ba9cd3ceb7 Mon Sep 17 00:00:00 2001 From: Paul Poulose Date: Sat, 19 Sep 2026 10:40:18 -0400 Subject: [PATCH 08/14] fix: address PR #23 review comments (round 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Idempotency/no-duplicate hardening found by CodeRabbit + Codex on the crash-recovery path (all panel-approved by Paul): - issue-payload: always keep the hidden recovery marker within the body cap (truncate content, then append marker) — a long body could previously drop the marker and defeat crash recovery -> duplicate. - github-client: treat GitHub 5xx on create as AMBIGUOUS (may have landed server-side) so the pending link is kept for recovery, not cleared+recreated. - github-client: recovery search restricted to `type:issue` (was matching PRs); an incomplete/underfetched search now reports unconfirmed instead of a false "no issue" that could trigger a duplicate recreate. - github-client: recognize 403 secondary-rate-limit bodies as RATE_LIMITED. - sync-to-github: refuse pending recovery when the adapter repo changed under the pending row (don't search/recreate in the wrong repo). - sync-to-github: stamp tracks.last_github_sync_at on every successful sync (watermark for T6 SYNC_DRIFT; migrations/005 column was unused). - sync-to-github: read track+items in one REPEATABLE READ snapshot so the payload/hash can't mix pre-/post-change state. - README: separate implemented vs dogfooded counts (github not yet dogfooded). Tests: +4 unit (5xx ambiguity, secondary-rate-limit, type:issue, incomplete search) and +2 integration (repo-changed-in-recovery, sync watermark). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01278w3fed8ZcXjHpKrFHWDw --- README.md | 11 +-- src/db/queries/tracks.ts | 14 ++++ src/github/github-client.ts | 56 ++++++++++----- src/mcp/tools/issue-payload.ts | 14 ++-- src/mcp/tools/sync-to-github.ts | 50 ++++++++++--- tests/integration/sync-to-github.test.ts | 21 ++++++ tests/unit/github-client.test.ts | 91 ++++++++++++++++++++---- tests/unit/issue-payload.test.ts | 6 +- 8 files changed, 210 insertions(+), 53 deletions(-) diff --git a/README.md b/README.md index ee226e5..0a9c8e1 100644 --- a/README.md +++ b/README.md @@ -16,12 +16,13 @@ harness — not just one vendor's tool. ## Status -Pre-release, v0.1.0. 12 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). -`kt_sync_to_github` is implemented as of T5.2 (2026-09-19): idempotent -one-way sync of a track to a GitHub Issue, offline-verified, with its one -remaining gate a real-repo dogfood run. The remaining 2 ship as stub +`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 diff --git a/src/db/queries/tracks.ts b/src/db/queries/tracks.ts index 893d64d..6570b1f 100644 --- a/src/db/queries/tracks.ts +++ b/src/db/queries/tracks.ts @@ -359,3 +359,17 @@ export async function trackExistsInProject( ]); return (result.rowCount ?? 0) > 0; } + +/** Stamps `tracks.last_github_sync_at = now()` after a successful GitHub + * sync (T5.2). 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): Promise { + await db.query(`UPDATE tracks SET last_github_sync_at = now() WHERE id = $1`, [trackId]); +} diff --git a/src/github/github-client.ts b/src/github/github-client.ts index 2c08ca7..ce711b4 100644 --- a/src/github/github-client.ts +++ b/src/github/github-client.ts @@ -53,20 +53,26 @@ export interface GitHubClientOptions { export type GitHubClientFactory = (token: string, opts: GitHubClientOptions) => GitHubClient; -function isRateLimited(status: number, headers: Headers): boolean { +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 response means the request reached GitHub, so a - * create that got any response did NOT create an issue (GitHub rejected - * it): ambiguous=false for every mapped status here. Only never-completed - * requests (timeout/network) are ambiguous, handled by the caller's catch. */ + * 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, @@ -78,17 +84,18 @@ function mapErrorResponse( 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)) + 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 client/server status fold into UNKNOWN with GitHub's own message. + // 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: false, + ambiguous: status >= 500, }; } @@ -198,17 +205,17 @@ export function createFetchGitHubClient(token: string, opts: GitHubClientOptions }, async findIssueByMarker(repo, marker) { - // Recovery-only. GitHub search is eventually consistent, so the caller - // must treat a null result together with the pending link's age, never - // as immediate proof no issue exists. - const q = encodeURIComponent(`repo:${repo} in:body "${marker}"`); - const res = await send('GET', `${GITHUB_API}/search/issues?q=${q}&per_page=10`); + // 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; - const items = - body && typeof body === 'object' && Array.isArray((body as Record).items) - ? ((body as Record).items as unknown[]) - : []; + 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; @@ -218,6 +225,19 @@ export function createFetchGitHubClient(token: string, opts: GitHubClientOptions } } } + // 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/issue-payload.ts b/src/mcp/tools/issue-payload.ts index b6ba853..c5aae97 100644 --- a/src/mcp/tools/issue-payload.ts +++ b/src/mcp/tools/issue-payload.ts @@ -71,7 +71,7 @@ export function buildIssuePayload(track: TrackForIssue, items: ItemForIssue[]): ? `\n\n_… ${ordered.length - shown.length} more item(s) not shown._` : ''; - const bodyFull = [ + const content = [ '_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}\``, @@ -79,13 +79,19 @@ export function buildIssuePayload(track: TrackForIssue, items: ItemForIssue[]): '### Items', checklist.length > 0 ? checklist : '_No items yet._', elided, - '', - trackMarker(track.id), ].join('\n'); + // The hidden recovery marker MUST survive truncation: if a very long body + // were cut with the marker at the end, crash recovery could not find the + // issue and would create a duplicate. So reserve room for the marker and + // truncate the content, never the marker. + const markerBlock = `\n\n${trackMarker(track.id)}`; + const room = Math.max(0, BODY_MAX - markerBlock.length); + const body = truncate(content, room) + markerBlock; + const payload: IssuePayload = { title, - body: truncate(bodyFull, BODY_MAX), + body, state: isDone ? 'closed' : 'open', }; if (isDone) { diff --git a/src/mcp/tools/sync-to-github.ts b/src/mcp/tools/sync-to-github.ts index 4224c08..2a022ed 100644 --- a/src/mcp/tools/sync-to-github.ts +++ b/src/mcp/tools/sync-to-github.ts @@ -24,12 +24,12 @@ import { syncToGithubInputSchema, type SyncToGithubInput } from '../../schemas/t import { runTool } from '../tool-helpers.js'; import { conflict, notFound } from '../errors.js'; import { findActiveProjectById } from '../../db/queries/projects.js'; -import { findTrackById } from '../../db/queries/tracks.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 { withTransaction } from '../../db/tx.js'; +import { withReadSnapshot, withTransaction } from '../../db/tx.js'; import { claimPendingLink, clearPendingLink, @@ -108,11 +108,23 @@ export async function syncToGithubService( // surfaces as INTERNAL_ERROR via runTool — never leaks the token). const token = decryptCredential(adapter.encrypted_credential, config.encryptionKey); - // 4. Deterministic payload + no-op hash. - const items = await listItemsByTrack(pool, track.id); + // 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) => { + const t = await findTrackById(c, input.project_id, input.track_id); + return { 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 payload = buildIssuePayload( - { id: track.id, title: track.title, status: track.status }, - items.map((i) => ({ + { id: snap.track.id, title: snap.track.title, status: snap.track.status }, + snap.items.map((i) => ({ title: i.title, status: i.status, sequence_position: i.sequence_position, @@ -149,7 +161,8 @@ export async function syncToGithubService( switch (decision.kind) { case 'noop': - return { ok: true }; + // Nothing to push, but we confirmed the issue is in sync -> watermark. + return okSynced(pool, track.id); case 'repo_changed': return { ok: false, @@ -169,6 +182,14 @@ export async function syncToGithubService( } } +/** Records the sync watermark and returns success. Called on every ok:true + * path (create, update, no-op, adopt) so tracks.last_github_sync_at always + * reflects the most recent confirmed sync (T5.2; SYNC_DRIFT input for T6). */ +async function okSynced(pool: Pool, trackId: string): Promise { + await withTransaction(pool, (c) => touchGithubSyncWatermark(c, trackId)); + return { ok: true }; +} + type Decision = | { kind: 'noop' } | { kind: 'repo_changed'; row: TrackExternalLinkRow } @@ -223,7 +244,7 @@ async function createFresh( 'GITHUB_UNKNOWN_ERROR: issue created but its link could not be finalized; reconcile manually', }; } - return { ok: true }; + return okSynced(pool, trackId); } async function updateExisting( @@ -253,7 +274,7 @@ async function updateExisting( await withTransaction(pool, (c) => updateLinkedContentHash(c, { trackId, adapterType: ADAPTER, contentHash: hash }), ); - return { ok: true }; + return okSynced(pool, trackId); } async function recoverPending( @@ -265,6 +286,15 @@ async function recoverPending( payload: GitHubIssuePayload, hash: string, ): 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. @@ -292,7 +322,7 @@ async function recoverPending( error: 'GITHUB_UNKNOWN_ERROR: adopted issue could not be finalized; reconcile manually', }; } - return { ok: true }; + return okSynced(pool, trackId); } // Marker not found. Search is eventually consistent, so only treat a miss diff --git a/tests/integration/sync-to-github.test.ts b/tests/integration/sync-to-github.test.ts index 134a7aa..0df761e 100644 --- a/tests/integration/sync-to-github.test.ts +++ b/tests/integration/sync-to-github.test.ts @@ -157,6 +157,10 @@ describe('kt_sync_to_github — create / update / no-op', () => { 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 () => { @@ -350,4 +354,21 @@ describe('kt_sync_to_github — durable creation-intent / recovery', () => { expect(state.createCalls).toHaveLength(0); 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 index d7efed2..650ccdf 100644 --- a/tests/unit/github-client.test.ts +++ b/tests/unit/github-client.test.ts @@ -103,37 +103,84 @@ describe('createFetchGitHubClient — request shape', () => { const res = await client().findIssueByMarker('o/r', marker); expect(res).toEqual({ ok: true, value: { number: '2', html_url: 'u2' } }); - expect(fetchMock.mock.calls[0]![0]).toContain('/search/issues?q='); + 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 no body matches', async () => { + it('findIssueByMarker returns null when a COMPLETE search finds no match', async () => { vi.stubGlobal( 'fetch', vi.fn(() => Promise.resolve( - resp(200, JSON.stringify({ items: [{ number: 1, html_url: 'u', body: 'nope' }] })), + 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]> = [ - [401, {}, 'GITHUB_AUTH_FAILED'], - [403, { 'x-ratelimit-remaining': '0' }, 'GITHUB_RATE_LIMITED'], - [403, { 'retry-after': '60' }, 'GITHUB_RATE_LIMITED'], - [403, {}, 'GITHUB_AUTH_FAILED'], - [404, {}, 'GITHUB_NOT_FOUND'], - [422, {}, 'GITHUB_UNKNOWN_ERROR'], - [429, {}, 'GITHUB_RATE_LIMITED'], - [500, {}, 'GITHUB_UNKNOWN_ERROR'], + 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] of cases) { - it(`maps HTTP ${status} -> ${prefix} (ambiguous=false, no token leak)`, async () => { + for (const [status, headers, prefix, ambiguous] of cases) { + it(`maps HTTP ${status} -> ${prefix} (ambiguous=${ambiguous}, no token leak)`, async () => { vi.stubGlobal( 'fetch', vi.fn(() => @@ -144,12 +191,26 @@ describe('createFetchGitHubClient — error mapping (PRD §4.13 prefixes)', () = expect(res.ok).toBe(false); if (!res.ok) { expect(res.error.startsWith(prefix)).toBe(true); - expect(res.ambiguous).toBe(false); + 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', diff --git a/tests/unit/issue-payload.test.ts b/tests/unit/issue-payload.test.ts index 44632f6..6e9c769 100644 --- a/tests/unit/issue-payload.test.ts +++ b/tests/unit/issue-payload.test.ts @@ -53,7 +53,7 @@ describe('buildIssuePayload', () => { expect(payload.title).toHaveLength(256); }); - it('truncates the body to GitHub’s 65,536-char cap', () => { + it('truncates the body to GitHub’s 65,536-char cap but always keeps the recovery marker', () => { const many = items( ...Array.from( { length: 400 }, @@ -62,6 +62,10 @@ describe('buildIssuePayload', () => { ); const payload = buildIssuePayload({ id: TRACK_ID, title: 'T', status: 'on_track' }, many); expect(payload.body.length).toBeLessThanOrEqual(65_536); + // Critical: an over-long body must NOT truncate away the marker, or crash + // recovery could not find the issue and would duplicate it. + expect(payload.body).toContain(trackMarker(TRACK_ID)); + expect(payload.body.endsWith(trackMarker(TRACK_ID))).toBe(true); }); it('caps the rendered checklist and notes the elision', () => { From 1708c9b71f671980e25d6da6373309a2eaafb28d Mon Sep 17 00:00:00 2001 From: Paul Poulose Date: Sat, 19 Sep 2026 10:49:43 -0400 Subject: [PATCH 09/14] fix: never auto-recreate on a recovery marker-miss (round 2) CodeRabbit correctly noted the aged-miss recreate still carried a residual duplicate risk: a complete GitHub Search miss cannot prove an earlier (ambiguous) create did not land, since search indexing can lag arbitrarily. That contradicted the chosen T5.2 contract (duplicate prevention takes priority over auto-retry when the outcome cannot be established). Recovery now ONLY adopts an issue the marker positively finds; a miss is never auto-recreated -- it returns an actionable error and leaves the pending link for explicit resolution (operator clear, or a future relink tool). Removed the age-window recreate path. Updated tests + TEST_CASES. Tradeoff: a track whose first sync failed ambiguously with no issue ever created stays pending until explicit resolution -- the accepted cost of the strict no-duplicate guarantee. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01278w3fed8ZcXjHpKrFHWDw --- docs/TEST_CASES.md | 2 +- src/mcp/tools/sync-to-github.ts | 52 +++++++----------------- tests/integration/sync-to-github.test.ts | 27 ++++-------- 3 files changed, 24 insertions(+), 57 deletions(-) diff --git a/docs/TEST_CASES.md b/docs/TEST_CASES.md index ad9fcde..4bfedb4 100644 --- a/docs/TEST_CASES.md +++ b/docs/TEST_CASES.md @@ -438,7 +438,7 @@ section tests the *detection logic itself*, which is triggered both inline by | 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 create | Same call | Recovers via the hidden `` marker — adopts the existing Issue if found, recreates only after an aged search-miss, asks the caller to retry on a fresh miss — never creating a duplicate | +| 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/src/mcp/tools/sync-to-github.ts b/src/mcp/tools/sync-to-github.ts index 2a022ed..52b9823 100644 --- a/src/mcp/tools/sync-to-github.ts +++ b/src/mcp/tools/sync-to-github.ts @@ -47,11 +47,6 @@ import { const ADAPTER = 'github' as const; const USER_AGENT = 'knotrack-mcp-server'; -// Below this age a `pending` link whose issue the marker search can't find -// might just be un-indexed (GitHub search is eventually consistent), so we -// ask the caller to retry rather than recreate. Past it, a search miss is -// strong enough evidence to clear and recreate. -const RECOVERY_INDEX_LAG_MS = 60_000; export interface GithubSyncOutput extends Record { ok: boolean; @@ -325,38 +320,21 @@ async function recoverPending( return okSynced(pool, trackId); } - // Marker not found. Search is eventually consistent, so only treat a miss - // as authoritative once the pending row is older than the index-lag window. - const ageMs = Date.now() - row.created_at.getTime(); - if (ageMs <= RECOVERY_INDEX_LAG_MS) { - return { - ok: false, - error: - 'GITHUB_UNKNOWN_ERROR: a previous sync did not complete and its outcome is not yet confirmable; retry shortly', - }; - } - const operationId = randomUUID(); - const reclaimed = await withTransaction(pool, async (c) => { - const cur = await getLinkForUpdate(c, trackId, ADAPTER); - if (!cur || cur.sync_state !== 'pending' || cur.operation_id !== row.operation_id) { - return false; - } - await clearPendingLink(c, { trackId, adapterType: ADAPTER, operationId: row.operation_id }); - const claimed = await claimPendingLink(c, { - trackId, - adapterType: ADAPTER, - repoIdentity: repo, - operationId, - }); - return claimed !== undefined; - }); - if (!reclaimed) { - return { - ok: false, - error: 'GITHUB_UNKNOWN_ERROR: pending sync changed during recovery; retry shortly', - }; - } - return createFresh(pool, client, repo, trackId, operationId, payload, hash); + // 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( diff --git a/tests/integration/sync-to-github.test.ts b/tests/integration/sync-to-github.test.ts index 0df761e..70663af 100644 --- a/tests/integration/sync-to-github.test.ts +++ b/tests/integration/sync-to-github.test.ts @@ -325,33 +325,22 @@ describe('kt_sync_to_github — durable creation-intent / recovery', () => { expect(row?.external_id).toBe('42'); }); - it('recreates when an aged pending link has no findable issue', async () => { + 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 minutes')`, + 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).toEqual({ ok: true }); - expect(state.findCalls).toHaveLength(1); - expect(state.createCalls).toHaveLength(1); // aged miss -> safe to recreate - expect((await linkRow(track_id))?.sync_state).toBe('linked'); - }); - - it('asks the caller to retry (no recreate) for a fresh pending link with no findable issue', async () => { - const { project_id, track_id } = await makeProjectTrack({ repo: REPO }); - 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(); - const res = await syncToGithubService(pool, config, { project_id, track_id }, deps); expect(res.ok).toBe(false); - expect(String(res.error)).toMatch(/retry shortly/); - expect(state.createCalls).toHaveLength(0); + 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'); }); From 1d772f52d28e97313ebd9c36308ae2602a56892d Mon Sep 17 00:00:00 2001 From: Paul Poulose Date: Sat, 19 Sep 2026 10:56:24 -0400 Subject: [PATCH 10/14] fix: address PR #23 Codex round-2 (malformed-JSON ambiguity, watermark, PRD) - github-client: a 2xx create whose body is malformed JSON is now AMBIGUOUS (the issue may have been created), so the pending link is kept for recovery rather than cleared+recreated. +unit test. - sync-to-github/tracks: stamp last_github_sync_at to the payload SNAPSHOT time, not the later completion time, so an item change after the snapshot still reads as drift for T6 SYNC_DRIFT (was now()). - PRD: remove stale stub-era claims (adapters unimplemented; last_github_sync_at not written) now that kt_sync_to_github ships and writes the watermark. The round-2 age-check duplicate finding was already resolved in 1708c9b (the aged-miss recreate was removed entirely). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01278w3fed8ZcXjHpKrFHWDw --- docs/PRD.md | 6 ++--- src/db/queries/tracks.ts | 17 ++++++++++--- src/github/github-client.ts | 5 +++- src/mcp/tools/sync-to-github.ts | 43 ++++++++++++++++++++++---------- tests/unit/github-client.test.ts | 13 ++++++++++ 5 files changed, 63 insertions(+), 21 deletions(-) diff --git a/docs/PRD.md b/docs/PRD.md index dbd895f..67e96b3 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. @@ -550,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. --- @@ -567,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/src/db/queries/tracks.ts b/src/db/queries/tracks.ts index 6570b1f..ba1544d 100644 --- a/src/db/queries/tracks.ts +++ b/src/db/queries/tracks.ts @@ -360,8 +360,9 @@ export async function trackExistsInProject( return (result.rowCount ?? 0) > 0; } -/** Stamps `tracks.last_github_sync_at = now()` after a successful GitHub - * sync (T5.2). The column was added in migrations/005 for exactly this +/** 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 @@ -370,6 +371,14 @@ export async function trackExistsInProject( * 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): Promise { - await db.query(`UPDATE tracks SET last_github_sync_at = now() WHERE id = $1`, [trackId]); +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 index ce711b4..3d92c6e 100644 --- a/src/github/github-client.ts +++ b/src/github/github-client.ts @@ -141,7 +141,10 @@ export function createFetchGitHubClient(token: string, opts: GitHubClientOptions return { ok: false, error: 'GITHUB_UNKNOWN_ERROR: malformed JSON in GitHub response', - ambiguous: false, + // The status was 2xx, so a create may already have succeeded — + // keep it ambiguous so the caller retains its pending link for + // recovery instead of clearing and recreating a duplicate. + ambiguous: true, }; } return { ok: true, value: { status: res.status, body } }; diff --git a/src/mcp/tools/sync-to-github.ts b/src/mcp/tools/sync-to-github.ts index 52b9823..e570a2d 100644 --- a/src/mcp/tools/sync-to-github.ts +++ b/src/mcp/tools/sync-to-github.ts @@ -107,8 +107,11 @@ export async function syncToGithubService( // 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 { track: t, items: t ? await listItemsByTrack(c, t.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. @@ -117,6 +120,7 @@ export async function syncToGithubService( 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) => ({ @@ -157,7 +161,7 @@ export async function syncToGithubService( switch (decision.kind) { case 'noop': // Nothing to push, but we confirmed the issue is in sync -> watermark. - return okSynced(pool, track.id); + return okSynced(pool, track.id, syncedAt); case 'repo_changed': return { ok: false, @@ -169,19 +173,29 @@ export async function syncToGithubService( 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); + 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); + 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); + return recoverPending(pool, client, repo, track.id, decision.row, payload, hash, syncedAt); } } -/** Records the sync watermark and returns success. Called on every ok:true - * path (create, update, no-op, adopt) so tracks.last_github_sync_at always - * reflects the most recent confirmed sync (T5.2; SYNC_DRIFT input for T6). */ -async function okSynced(pool: Pool, trackId: string): Promise { - await withTransaction(pool, (c) => touchGithubSyncWatermark(c, trackId)); +/** 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 }; } @@ -201,6 +215,7 @@ async function createFresh( operationId: string, payload: GitHubIssuePayload, hash: string, + syncedAt: Date, ): Promise { const created = await client.createIssue(repo, payload); if (!created.ok) { @@ -239,7 +254,7 @@ async function createFresh( 'GITHUB_UNKNOWN_ERROR: issue created but its link could not be finalized; reconcile manually', }; } - return okSynced(pool, trackId); + return okSynced(pool, trackId, syncedAt); } async function updateExisting( @@ -250,6 +265,7 @@ async function updateExisting( row: TrackExternalLinkRow, payload: GitHubIssuePayload, hash: string, + syncedAt: Date, ): Promise { // row is linked -> external_id is the issue number. const issueNumber = row.external_id; @@ -269,7 +285,7 @@ async function updateExisting( await withTransaction(pool, (c) => updateLinkedContentHash(c, { trackId, adapterType: ADAPTER, contentHash: hash }), ); - return okSynced(pool, trackId); + return okSynced(pool, trackId, syncedAt); } async function recoverPending( @@ -280,6 +296,7 @@ async function recoverPending( 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. @@ -317,7 +334,7 @@ async function recoverPending( error: 'GITHUB_UNKNOWN_ERROR: adopted issue could not be finalized; reconcile manually', }; } - return okSynced(pool, trackId); + return okSynced(pool, trackId, syncedAt); } // Marker not found. A pending link only survives an AMBIGUOUS create diff --git a/tests/unit/github-client.test.ts b/tests/unit/github-client.test.ts index 650ccdf..17cd28d 100644 --- a/tests/unit/github-client.test.ts +++ b/tests/unit/github-client.test.ts @@ -224,6 +224,19 @@ describe('createFetchGitHubClient — error mapping (PRD §4.13 prefixes)', () = } }); + 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', From 414ad24409f726485f8551d90271d0fc280a8e02 Mon Sep 17 00:00:00 2001 From: Paul Poulose Date: Sat, 19 Sep 2026 10:57:50 -0400 Subject: [PATCH 11/14] fix: guard 009 down-migration row-check when table already absent to_regclass check before SELECT ... FROM track_external_links, so a re-run rollback (table already dropped) does not raise undefined_table before the idempotent DROP. CodeRabbit round-3 nit. Down-only; not exercised by CI. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01278w3fed8ZcXjHpKrFHWDw --- migrations/009_track_external_links.down.sql | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/migrations/009_track_external_links.down.sql b/migrations/009_track_external_links.down.sql index bca171c..374a1c7 100644 --- a/migrations/009_track_external_links.down.sql +++ b/migrations/009_track_external_links.down.sql @@ -11,9 +11,14 @@ BEGIN; DO $$ BEGIN - 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.'; + -- 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 $$; From 6a5eca17451774c31e8e444db377be9500d3c06b Mon Sep 17 00:00:00 2001 From: Paul Poulose Date: Sat, 19 Sep 2026 11:15:27 -0400 Subject: [PATCH 12/14] fix: harden issue-payload rendering (marker injection, truncation notice, order) Three Codex P2 findings on issue-payload.ts: - Sanitize user-controlled titles (item + track) so an embedded cannot inject a fake recovery marker and let findIssueByMarker match/overwrite an unrelated issue (HTML-escape the comment delimiters). - Build the checklist WITHIN the body byte budget and always emit an accurate 'N more not shown' notice (was: long titles under the 300-item cap could be silently byte-truncated with no notice, and changes past the cut did not change the hash). - Deterministic order by (sequence_position, id): sequence_position is not unique, so ties previously left order to Postgres -> flapping body/hash and needless PATCHes. +3 unit tests. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01278w3fed8ZcXjHpKrFHWDw --- src/mcp/tools/issue-payload.ts | 68 ++++++++++++++++++++++++-------- src/mcp/tools/sync-to-github.ts | 1 + tests/unit/issue-payload.test.ts | 43 +++++++++++++++++++- 3 files changed, 94 insertions(+), 18 deletions(-) diff --git a/src/mcp/tools/issue-payload.ts b/src/mcp/tools/issue-payload.ts index c5aae97..f0948fc 100644 --- a/src/mcp/tools/issue-payload.ts +++ b/src/mcp/tools/issue-payload.ts @@ -25,6 +25,10 @@ export interface TrackForIssue { } 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; @@ -55,39 +59,69 @@ 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 { - const title = truncate(track.title, TITLE_MAX); + const title = sanitize(truncate(track.title, TITLE_MAX)); const isDone = track.status === 'done'; - const ordered = [...items].sort((a, b) => a.sequence_position - b.sequence_position); - const shown = ordered.slice(0, ITEM_RENDER_CAP); - const checklist = shown.map((i) => `- ${itemCheckbox(i.status)} ${i.title}`).join('\n'); - const elided = - ordered.length > shown.length - ? `\n\n_… ${ordered.length - shown.length} more item(s) not shown._` - : ''; + // 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 content = [ + 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', - checklist.length > 0 ? checklist : '_No items yet._', - elided, + '', ].join('\n'); - // The hidden recovery marker MUST survive truncation: if a very long body - // were cut with the marker at the end, crash recovery could not find the - // issue and would create a duplicate. So reserve room for the marker and - // truncate the content, never the marker. + // 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 room = Math.max(0, BODY_MAX - markerBlock.length); - const body = truncate(content, room) + markerBlock; + 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, diff --git a/src/mcp/tools/sync-to-github.ts b/src/mcp/tools/sync-to-github.ts index e570a2d..b9f0db5 100644 --- a/src/mcp/tools/sync-to-github.ts +++ b/src/mcp/tools/sync-to-github.ts @@ -124,6 +124,7 @@ export async function syncToGithubService( 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, diff --git a/tests/unit/issue-payload.test.ts b/tests/unit/issue-payload.test.ts index 6e9c769..9f5a933 100644 --- a/tests/unit/issue-payload.test.ts +++ b/tests/unit/issue-payload.test.ts @@ -9,7 +9,12 @@ import { const TRACK_ID = '11111111-1111-4111-8111-111111111111'; function items(...specs: Array<[string, string, number]>): ItemForIssue[] { - return specs.map(([title, status, sequence_position]) => ({ title, status, sequence_position })); + return specs.map(([title, status, sequence_position], i) => ({ + id: `item-${i}`, + title, + status, + sequence_position, + })); } describe('buildIssuePayload', () => { @@ -78,6 +83,42 @@ describe('buildIssuePayload', () => { const payload = buildIssuePayload({ id: TRACK_ID, title: 'T', status: 'on_track' }, many); expect(payload.body).toMatch(/more item\(s\) not shown/); }); + + it('neutralizes a fake recovery marker embedded in an item or track title', () => { + const OTHER = '22222222-2222-4222-8222-222222222222'; + const fake = trackMarker(OTHER); // another track's exact marker + const payload = buildIssuePayload( + { id: TRACK_ID, title: `Sneaky ${fake}`, status: 'on_track' }, + items([`item ${fake}`, 'pending', 1]), + ); + // the ONLY genuine marker present is this track's own (appended by us); + // the injected one must be broken so it cannot be marker-matched. + expect(payload.body).toContain(trackMarker(TRACK_ID)); + expect(payload.body).not.toContain(fake); + }); + + it('reports omitted items when the body is byte-truncated below the item cap', () => { + // 250 items (< 300 cap) but huge titles -> byte budget cuts the tail + const many = items( + ...Array.from( + { length: 250 }, + (_, i) => ['z'.repeat(400), 'pending', i] as [string, string, number], + ), + ); + const payload = buildIssuePayload({ id: TRACK_ID, title: 'T', status: 'on_track' }, many); + expect(payload.body.length).toBeLessThanOrEqual(65_536); + expect(payload.body).toMatch(/more item\(s\) not shown/); // not silently partial + expect(payload.body).toContain(trackMarker(TRACK_ID)); + }); + + it('orders items deterministically by (sequence_position, id) so the hash is stable', () => { + const a: ItemForIssue = { id: 'aaa', title: 'A', status: 'pending', sequence_position: 1 }; + const b: ItemForIssue = { id: 'bbb', title: 'B', status: 'pending', sequence_position: 1 }; + const forward = buildIssuePayload({ id: TRACK_ID, title: 'T', status: 'on_track' }, [a, b]); + const reversed = buildIssuePayload({ id: TRACK_ID, title: 'T', status: 'on_track' }, [b, a]); + expect(payloadContentHash(forward)).toBe(payloadContentHash(reversed)); + expect(forward.body.indexOf('A')).toBeLessThan(forward.body.indexOf('B')); + }); }); describe('payloadContentHash', () => { From 938b2e22aef7128e106022e2ae65fea6858d1106 Mon Sep 17 00:00:00 2001 From: Paul Poulose Date: Sat, 19 Sep 2026 11:24:55 -0400 Subject: [PATCH 13/14] fix: method-aware malformed-JSON ambiguity; document update-concurrency decision - github-client: a 2xx WRITE (POST/PATCH) with malformed JSON stays ambiguous (create may have landed); a GET (recovery search) does not. Refines the earlier blanket fix (1d772f5) per CodeRabbit. - Update-path concurrency: made the deliberate 'no lock across the GitHub network call' decision durable -- a code comment in updateExisting and a 'considered + deferred' note in ROADMAP T5.2 (create path is fully serialized; concurrent updates PATCH idempotently and self-correct). We declined CodeRabbit's cross-call lock (pool-exhaustion risk on a self-hosted server); revisit only if real contention is observed. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01278w3fed8ZcXjHpKrFHWDw --- docs/ROADMAP.md | 15 ++++++++++++++- src/github/github-client.ts | 10 ++++++---- src/mcp/tools/sync-to-github.ts | 8 ++++++++ 3 files changed, 28 insertions(+), 5 deletions(-) diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index f2c2a4e..59085dc 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -1004,7 +1004,20 @@ corrected track-status model, not the currently-broken one). (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). The **one + 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`. diff --git a/src/github/github-client.ts b/src/github/github-client.ts index 3d92c6e..4c7c4e3 100644 --- a/src/github/github-client.ts +++ b/src/github/github-client.ts @@ -141,10 +141,12 @@ export function createFetchGitHubClient(token: string, opts: GitHubClientOptions return { ok: false, error: 'GITHUB_UNKNOWN_ERROR: malformed JSON in GitHub response', - // The status was 2xx, so a create may already have succeeded — - // keep it ambiguous so the caller retains its pending link for - // recovery instead of clearing and recreating a duplicate. - ambiguous: true, + // 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 } }; diff --git a/src/mcp/tools/sync-to-github.ts b/src/mcp/tools/sync-to-github.ts index b9f0db5..3b8e236 100644 --- a/src/mcp/tools/sync-to-github.ts +++ b/src/mcp/tools/sync-to-github.ts @@ -276,6 +276,14 @@ async function updateExisting( 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 From 0de87b8492c44897319de671a1b87159590c8c84 Mon Sep 17 00:00:00 2001 From: Paul Poulose Date: Sat, 19 Sep 2026 11:44:44 -0400 Subject: [PATCH 14/14] fix: enforce title cap after escaping; finish PRD github-stub cleanup Codex PR #23 round-3: - issue-payload: escape delimiters BEFORE truncating so a near-limit title full of `` markers cannot expand past GitHub's 256-char cap and 422 the sync. Adds a regression test. - PRD: drop the two stale claims that kt_sync_to_github is not yet implemented (6 Success Metrics, 7 Out of Scope). GitHub sync IS live in this build; only the Linear adapter (4.14) remains a stub. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01278w3fed8ZcXjHpKrFHWDw --- docs/PRD.md | 4 ++-- src/mcp/tools/issue-payload.ts | 5 ++++- tests/unit/issue-payload.test.ts | 11 +++++++++++ 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/docs/PRD.md b/docs/PRD.md index 67e96b3..067b37b 100644 --- a/docs/PRD.md +++ b/docs/PRD.md @@ -524,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. --- @@ -534,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. diff --git a/src/mcp/tools/issue-payload.ts b/src/mcp/tools/issue-payload.ts index f0948fc..352b3a7 100644 --- a/src/mcp/tools/issue-payload.ts +++ b/src/mcp/tools/issue-payload.ts @@ -74,7 +74,10 @@ function sanitize(value: string): string { * 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 { - const title = sanitize(truncate(track.title, TITLE_MAX)); + // 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 diff --git a/tests/unit/issue-payload.test.ts b/tests/unit/issue-payload.test.ts index 9f5a933..b4d5c2c 100644 --- a/tests/unit/issue-payload.test.ts +++ b/tests/unit/issue-payload.test.ts @@ -58,6 +58,17 @@ describe('buildIssuePayload', () => { 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('