From 1384e25450f76fbb44a7e0fff11c233c9f656534 Mon Sep 17 00:00:00 2001 From: Paul Poulose Date: Mon, 24 Aug 2026 07:40:38 -0400 Subject: [PATCH 1/3] docs: consolidate untracked review findings into ROADMAP.md backlog The v1 adversarial review (run-20260823-020205) confirmed 11 medium/low findings as real but deferred them to "backlog" without ever writing them down anywhere outside the review run's own artifacts -- only reliability-2 (DB retry/backoff) actually made it into ROADMAP.md. This left security-3/4/5, correctness-2/3, test_quality-1 through -5, and reliability-6 confirmed-real but effectively untracked. Similarly, PR #1's three deliberately-deferred CodeRabbit findings (track unblock path, MCP SDK v2 migration, migration rollback mode) and the newly-surfaced remaining root_path/repo_url/adapters_enabled doc drift (kt_get_project_status, the roadmap/sync stub tools, and the glossary) existed only in review comments and conversation notes, not in the repo. Added all of the above to ROADMAP.md's Backlog section, in the same style as the existing "Deferred from the v1 adversarial review" entries, each citing its source finding id and file so it can be found again. Also noted the single-shared-token trust model (security-1/security-3 from the initial panel pass) as accepted risk, not a backlog item -- it's documented v1 design per TRD.md, not an omission -- since that distinction wasn't written down anywhere either. Docs-only change; no code touched. --- docs/ROADMAP.md | 103 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 100 insertions(+), 3 deletions(-) diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index 016c9e1..ec3b8ea 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -452,8 +452,10 @@ partway through this build. Recorded here so the decisions aren't re-litigated l deterministic, self-hosted, no-daemon design already settled on (see `ARCHITECTURE.md` §6, the anti-orchestrator argument). Rejected. -**Deferred from the v1 adversarial review (see `suppressions.json` in the -review run for the full technical justification and expiry dates):** +**Deferred from the v1 adversarial review (see `.adversarial-review/run-20260823-020205/report.md` +and `suppressions.json` in that run for full technical justification, reproduction notes, and +expiry dates). All items below were confirmed real by direct code reading during that review — +none are guesses — but judged disproportionate to fix reactively under review pressure:** - **`T9.x` (new, unscheduled) — DB-operation retry/backoff.** No service function retries a transient DB failure (connection reset, serialization error) today; a failure just fails fast and rolls back cleanly (the @@ -462,4 +464,99 @@ review run for the full technical justification and expiry dates):** generic retry layer needs to reason about which operations are safe to retry blindly, which is a bigger, cross-cutting change than fits reactively inside one review; tracked here instead of built ad hoc. - Suppression expires 2026-11-23 — revisit before then. + Suppression expires 2026-11-23 — revisit before then. (Findings + `reliability-2`, `reliability-5`.) +- **`T9.x` (new, unscheduled) — bound `files_touched` and adapter-secret + input sizes.** `kt_record_session_summary`'s `files_touched` array and + `kt_register_project`'s `adapters.*` credential strings have no `.max()` + bound in `src/schemas/tools.ts` — an authenticated caller can submit + arbitrarily large payloads, a storage/memory DoS vector. Needs a handful + of Zod `.max()` additions; small but not done reactively under review. + (Finding `security-3`.) +- **`T9.x` (new, unscheduled) — stop echoing raw exception text to + clients.** `register-project.ts`'s error path puts the raw driver/crypto + exception message on `details.cause`, which `runTool` serializes verbatim + into the client-facing error envelope — a low-severity internals leak. + Needs the same "log full detail server-side, return a generic message" + treatment already used elsewhere. (Finding `security-4`.) +- **`T9.x` (new, unscheduled) — stop disclosing Node.js version on + unauthenticated `/info`.** Low-severity recon surface for an attacker + fingerprinting the server. (Finding `security-5`.) +- **`T9.x` (new, unscheduled) — close `hasOpenFlagForItem`'s TOCTOU race.** + Same check-then-insert race family as `correctness-1` (already fixed for + sequence-position auto-assign via `SELECT ... FOR UPDATE`), but for + drift-flag dedup: concurrent `record_session_summary` calls can insert + duplicate open flags for the same item. Likely closes with the same + partial-unique-index pattern used for `correctness-1`'s fix. (Finding + `correctness-2`.) +- **Needs a product decision, not a code fix — `findSequenceSkips`' + "complete" definition.** The reviewer flagged (confidence 0.3, genuinely + ambiguous against TRD text) that `findSequenceSkips` treats an item with + status `in_progress` as "complete" for ordering purposes. Before this is + actionable, Paul needs to say whether `in_progress` should count as + complete for sequence-skip detection or not — this is a semantics + question, not a bug with an obvious fix. (Finding `correctness-3`.) +- **`T9.x` (new, unscheduled) — five test-coverage gaps, no behavior + change.** (1) `kt_record_session_summary`: no 404 test for + `items_touched` referencing a nonexistent item id. (2) `runTool`'s error + envelope shape and generic-error redaction has no direct unit test. (3) + `kt_register_project`'s Linear-adapter credential encryption path is + untested (only GitHub's is). (4) `kt_get_project_status`: no test + verifies `drift_flags` field mapping when open flags actually exist. (5) + No test verifies a soft-deleted project (`deleted_at` set) is rejected by + every service function, not just some. All confirmed real gaps, all + test-only additions. (Findings `test_quality-1` through `test_quality-5`.) +- **`T9.x` (new, unscheduled) — structured logging for drift-scan + outcomes.** No log line records drift-scan duration, item count scanned, + or flags raised per `kt_record_session_summary` call — an observability + gap, not a correctness one. (Finding `reliability-6`.) + +**Accepted risk, not a backlog item — the single shared-token trust model.** +Findings `security-1` and `security-3` from the *initial* panel pass (distinct +from the `security-3` above, which is from the final rerun and reused the same +id) argued that any valid bearer token can read/write any project on the +instance, including rotating another project's adapter credentials via +`kt_register_project`'s upsert. `docs/TRD.md` §4/§7 document this as the +deliberate v1 trust boundary — one instance, one operator, one trust domain; +isolating two teams means running two instances — not an omission. Suppression +expires 2027-02-23. Revisit only if a genuine multi-tenant (multiple mutually +untrusting operators on one instance) need shows up; until then, nothing to +build. + +**Deferred from PR #1's CodeRabbit review round (2026-08-23/24) — architecture/ +scope decisions, not bugs, per the `clear-decisions` walkthrough:** +- **`T9.x` (new, unscheduled) — a path to unblock a `blocked` track.** + `kt_create_track` can set a track to `blocked` when a declared dependency + isn't `done`, but there is no write path from `blocked` back to + `on_track` or `done` once created — dependents stay blocked permanently. + Deferred until a real caller actually hits this (no current tool + triggers the transition back). +- **`T9.x` (new, unscheduled) — `@modelcontextprotocol/sdk` v1 → v2 + migration.** Needed for `server/discover` and the 2026-07-28 protocol + revision's stateless discovery mechanism. Breaking (modular v2 packages, + ESM-first, Zod `^4.2.0` requirement), touches every tool registration and + the core transport. Deferred indefinitely — no current forcing need. +- **`T9.x` (new, unscheduled) — migration rollback (`down`) mode.** + `scripts/migrate.ts` only applies migrations forward; there's no guarded + mode to run `.down.sql` files in reverse and remove their ledger entries. + Note this isn't just a TRD gap: `T2.1` above states `migrate down` + cleanly reverses `001_init.sql` as its own acceptance criterion, so this + was always intended, not merely documented aspirationally. Deferred + until a real forward migration actually needs reverting. +- **`T9.x` (new, unscheduled) — finish propagating the source_type/ + source_ref registration model through the remaining docs.** Commits + `5809ae4`/`bca45ff` fixed the "13 tools"/ID-format/cross-track-deps/ + credential-contract drift in `PRD.md` sections 4.1, 4.4, 4.7, 5.3, and + the Appendix, but did not touch: `kt_get_project_status`'s own PRD + section (still describes a `{ project: { root_path, repo_url, + adapters_enabled } }` shape that `get-project-status.ts` doesn't return + at all — flagged as its own separate drift in commit `208c90b`, not + re-verified this round); `kt_render_roadmap`, `kt_sync_to_github`, and + `kt_sync_to_linear`'s PRD sections (all three are unimplemented v1 stubs + — `src/mcp/tools/stubs.ts` — so there's no real code yet to verify the + `root_path`/`repo_url`/`adapters_enabled`/`GITHUB_TOKEN`/`LINEAR_API_KEY` + prose against); and one stray `root_path`/`repo_url` mention in PRD.md + §6's glossary. Needs a pass once `kt_get_project_status` is re-verified + against real code and/or the sync tools get real implementations — + documenting the sync tools' contract now would be speculative ahead of + building them. From d41b54c2328754fbd7687874465087b7411e2ce8 Mon Sep 17 00:00:00 2001 From: Paul Poulose Date: Mon, 24 Aug 2026 07:57:47 -0400 Subject: [PATCH 2/3] docs: fix self-contradictions CodeRabbit caught in the new ROADMAP.md backlog section Three minor issues in the just-added backlog section, all genuine: - Claimed .adversarial-review/run-20260823-020205/report.md and suppressions.json were linkable evidence, but .adversarial-review/ is gitignored and was never committed -- a Markdown link there would be dead for anyone browsing the repo or cloning fresh. Reworded to say so explicitly instead of linking to a path that doesn't exist in-repo. - The intro claimed every listed item was "confirmed real ... none are guesses," but the very next entries include correctness-3, which is explicitly described (by this same section) as confidence 0.3 and genuinely ambiguous. Added the exception. - Reused the id "security-3" for two unrelated findings (input-size limits from the final rerun vs. the shared-token trust model from the initial panel pass) with no way to tell them apart. Disambiguated both mentions. Docs-only; no code touched. --- docs/ROADMAP.md | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index ec3b8ea..50764b9 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -452,10 +452,13 @@ partway through this build. Recorded here so the decisions aren't re-litigated l deterministic, self-hosted, no-daemon design already settled on (see `ARCHITECTURE.md` §6, the anti-orchestrator argument). Rejected. -**Deferred from the v1 adversarial review (see `.adversarial-review/run-20260823-020205/report.md` -and `suppressions.json` in that run for full technical justification, reproduction notes, and -expiry dates). All items below were confirmed real by direct code reading during that review — -none are guesses — but judged disproportionate to fix reactively under review pressure:** +**Deferred from the v1 adversarial review (see `report.md` and `suppressions.json` under +`.adversarial-review/run-20260823-020205/` for full technical justification, reproduction notes, +and expiry dates — that directory is gitignored and local to the machine the review ran on, not +committed to this repo, so there is no in-repo link to give it; re-run the `adversarial-review` +skill to regenerate it if needed). All items below were confirmed real by direct code reading +during that review — except `correctness-3`, explicitly flagged low-confidence and ambiguous, see +its own entry below — and judged disproportionate to fix reactively under review pressure:** - **`T9.x` (new, unscheduled) — DB-operation retry/backoff.** No service function retries a transient DB failure (connection reset, serialization error) today; a failure just fails fast and rolls back cleanly (the @@ -472,7 +475,7 @@ none are guesses — but judged disproportionate to fix reactively under review bound in `src/schemas/tools.ts` — an authenticated caller can submit arbitrarily large payloads, a storage/memory DoS vector. Needs a handful of Zod `.max()` additions; small but not done reactively under review. - (Finding `security-3`.) + (Finding `security-3`, final rerun.) - **`T9.x` (new, unscheduled) — stop echoing raw exception text to clients.** `register-project.ts`'s error path puts the raw driver/crypto exception message on `details.cause`, which `runTool` serializes verbatim @@ -512,9 +515,10 @@ none are guesses — but judged disproportionate to fix reactively under review gap, not a correctness one. (Finding `reliability-6`.) **Accepted risk, not a backlog item — the single shared-token trust model.** -Findings `security-1` and `security-3` from the *initial* panel pass (distinct -from the `security-3` above, which is from the final rerun and reused the same -id) argued that any valid bearer token can read/write any project on the +Findings `security-1` (initial panel pass) and `security-3` (initial panel +pass — a *different* finding than the `security-3, final rerun` above; the +two review passes independently reused the same id number for unrelated +findings) argued that any valid bearer token can read/write any project on the instance, including rotating another project's adapter credentials via `kt_register_project`'s upsert. `docs/TRD.md` §4/§7 document this as the deliberate v1 trust boundary — one instance, one operator, one trust domain; From d8ae823e119bb04f4f652400215edbd67ca2bc3c Mon Sep 17 00:00:00 2001 From: Paul Poulose Date: Mon, 24 Aug 2026 08:11:03 -0400 Subject: [PATCH 3/3] docs: remove three ROADMAP.md backlog items already fixed/resolved, fix stub-schema claim ChatGPT Codex reviewed PR #2's ROADMAP.md addition and flagged 5 issues. Verified each against current code before acting, per the same verify-before-fix discipline used all along this review loop: - security-4 (raw exception leak in register-project.ts): stale. The catch blocks already throw a fixed internalError() message, not details.cause -- confirmed by direct code read and by tests/integration/register-project.test.ts asserting the crypto error text is absent from the envelope. Removed the backlog item. - correctness-2 (hasOpenFlagForItem TOCTOU race): stale. Already closed by migrations/003_drift_flags_open_unique.sql's partial unique index plus insertDriftFlagIfNotOpen's ON CONFLICT ... DO NOTHING in src/db/queries/drift-flags.ts. Removed the backlog item. - correctness-3 (findSequenceSkips' in_progress handling): the original reviewer flagged this at confidence 0.3 specifically because TRD text was excluded from its diff context (see report.md's known-limitations section). Read docs/TRD.md Appendix B directly: SEQUENCE_SKIP is unambiguously defined as an earlier item in pending or blocked (not in_progress). findSequenceSkips in src/domain/drift-detector.ts implements that exact predicate. Not ambiguous -- removed the backlog item instead of leaving it blocked on a product decision that doesn't need making. - Dead-link wording (report.md/suppressions.json unreachable): reworded the section intro so each bullet's inline justification stands on its own, rather than reading as if the gitignored, uncommitted run artifacts are necessary to understand the backlog. - Stub-tool schema claim: was wrong to call this speculative. All three stub tools (kt_render_roadmap, kt_sync_to_github, kt_sync_to_linear) already register real Zod schemas via src/mcp/tools/stubs.ts, exposed through tools/list today. Those schemas already contradict the PRD prose right now (kt_render_roadmap has no output_path field; the sync tools have no adapters_enabled field) -- rewrote the entry to say so concretely instead of deferring on "no real code yet." All five verified by direct code/test reading before editing; none required more than confirming or correcting this backlog's own wording. --- docs/ROADMAP.md | 62 +++++++++++++++++++++---------------------------- 1 file changed, 27 insertions(+), 35 deletions(-) diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index 50764b9..5d7849b 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -452,13 +452,15 @@ partway through this build. Recorded here so the decisions aren't re-litigated l deterministic, self-hosted, no-daemon design already settled on (see `ARCHITECTURE.md` §6, the anti-orchestrator argument). Rejected. -**Deferred from the v1 adversarial review (see `report.md` and `suppressions.json` under -`.adversarial-review/run-20260823-020205/` for full technical justification, reproduction notes, -and expiry dates — that directory is gitignored and local to the machine the review ran on, not -committed to this repo, so there is no in-repo link to give it; re-run the `adversarial-review` -skill to regenerate it if needed). All items below were confirmed real by direct code reading -during that review — except `correctness-3`, explicitly flagged low-confidence and ambiguous, see -its own entry below — and judged disproportionate to fix reactively under review pressure:** +**Deferred from the v1 adversarial review — each item below carries its own justification +inline, re-verified against current code as of this entry rather than merely restated from the +review. The review run's `report.md` and `suppressions.json` (under +`.adversarial-review/run-20260823-020205/`) hold a fuller per-finding evidence trail — exact +repro steps, severity scoring, panel votes — but that directory is gitignored and local to the +machine the review ran on, not committed to this repo (re-run the `adversarial-review` skill to +regenerate it if that extra detail is ever needed); nothing below depends on it being available. +All items were confirmed real by direct code reading and judged disproportionate to fix +reactively under review pressure:** - **`T9.x` (new, unscheduled) — DB-operation retry/backoff.** No service function retries a transient DB failure (connection reset, serialization error) today; a failure just fails fast and rolls back cleanly (the @@ -476,29 +478,9 @@ its own entry below — and judged disproportionate to fix reactively under revi arbitrarily large payloads, a storage/memory DoS vector. Needs a handful of Zod `.max()` additions; small but not done reactively under review. (Finding `security-3`, final rerun.) -- **`T9.x` (new, unscheduled) — stop echoing raw exception text to - clients.** `register-project.ts`'s error path puts the raw driver/crypto - exception message on `details.cause`, which `runTool` serializes verbatim - into the client-facing error envelope — a low-severity internals leak. - Needs the same "log full detail server-side, return a generic message" - treatment already used elsewhere. (Finding `security-4`.) - **`T9.x` (new, unscheduled) — stop disclosing Node.js version on unauthenticated `/info`.** Low-severity recon surface for an attacker fingerprinting the server. (Finding `security-5`.) -- **`T9.x` (new, unscheduled) — close `hasOpenFlagForItem`'s TOCTOU race.** - Same check-then-insert race family as `correctness-1` (already fixed for - sequence-position auto-assign via `SELECT ... FOR UPDATE`), but for - drift-flag dedup: concurrent `record_session_summary` calls can insert - duplicate open flags for the same item. Likely closes with the same - partial-unique-index pattern used for `correctness-1`'s fix. (Finding - `correctness-2`.) -- **Needs a product decision, not a code fix — `findSequenceSkips`' - "complete" definition.** The reviewer flagged (confidence 0.3, genuinely - ambiguous against TRD text) that `findSequenceSkips` treats an item with - status `in_progress` as "complete" for ordering purposes. Before this is - actionable, Paul needs to say whether `in_progress` should count as - complete for sequence-skip detection or not — this is a semantics - question, not a bug with an obvious fix. (Finding `correctness-3`.) - **`T9.x` (new, unscheduled) — five test-coverage gaps, no behavior change.** (1) `kt_record_session_summary`: no 404 test for `items_touched` referencing a nonexistent item id. (2) `runTool`'s error @@ -556,11 +538,21 @@ scope decisions, not bugs, per the `clear-decisions` walkthrough:** adapters_enabled } }` shape that `get-project-status.ts` doesn't return at all — flagged as its own separate drift in commit `208c90b`, not re-verified this round); `kt_render_roadmap`, `kt_sync_to_github`, and - `kt_sync_to_linear`'s PRD sections (all three are unimplemented v1 stubs - — `src/mcp/tools/stubs.ts` — so there's no real code yet to verify the - `root_path`/`repo_url`/`adapters_enabled`/`GITHUB_TOKEN`/`LINEAR_API_KEY` - prose against); and one stray `root_path`/`repo_url` mention in PRD.md - §6's glossary. Needs a pass once `kt_get_project_status` is re-verified - against real code and/or the sync tools get real implementations — - documenting the sync tools' contract now would be speculative ahead of - building them. + `kt_sync_to_linear`'s PRD sections §4.12–4.14; and one stray + `root_path`/`repo_url` mention in PRD.md §6's glossary. This is not + speculative work waiting on real handlers: all three tools are + unimplemented stubs, but `src/mcp/tools/stubs.ts` registers each one's + real Zod schema from `src/schemas/tools.ts`, and `tools/list` already + publishes those schemas to clients today — so the contradiction with + the PRD prose is live now, not merely anticipated. Concretely: + `kt_render_roadmap`'s schema takes `project_id` + `format` + (`'markdown' | 'mermaid'`) with no `output_path` field at all, while + PRD §4.12's acceptance criteria are written entirely around a + `root_path`/`output_path` file-writing behavior the schema has no way + to express; `kt_sync_to_github` and `kt_sync_to_linear` both take only + `project_id` + `track_id`, while PRD §4.13/§4.14 gate on + `adapters_enabled`, a field the current `source_type`/`source_ref` + registration model (PRD §4.1) no longer has. Needs a pass once + `kt_get_project_status` is re-verified against real code — but the + three stub sections' prose can and should be fixed now, against the + schemas already shipping.