mirror of
https://github.com/Kaelio/ktx.git
synced 2026-06-13 08:15:14 +02:00
fix(cli): isolate ktx-owned project repositories (#283)
* fix(cli): isolate ktx project git repos * fix(cli): remove inert auto commit config * test(cli): drop stale auto commit fixtures * docs: document isolated ktx project repos * test(cli): keep stale config grep clean * fix(cli): guide setup away from foreign repos at the project dir ktx owns the git repo rooted at the project dir and refuses to adopt one it did not create (the Finding 3 isolation invariant). But setup steered users straight into that failure: the interactive menu offers "Current directory" first, and `--no-input --yes --project-dir <repo-root>` created directly in place — both then threw a generic "Failed to initialize git repository:" wrapper from deep in GitService.initialize(). Extract the ownership rule into a shared `classifyKtxRepoOwnership(dir)` used by both GitService.initialize() (the invariant) and the setup wizard (pre-flight guidance), so the decision derives from one rule. Setup now detects a foreign repo before constructing GitService and: interactively re-prompts (the user picks the existing `ktx-project` subfolder), or non-interactively returns a clean missing-input with the actionable message. The typed foreign-repo error is also surfaced verbatim instead of being buried under the generic wrapper. Empty/non-repo current directories still work — only foreign repos are blocked. * fix(cli): keep classifyKtxRepoOwnership total for non-directory paths The setup ownership guard runs before the existing not-a-directory check, so pointing a custom/--project-dir path at a file made classifyKtxRepoOwnership lstat `<file>/.git`, hit ENOTDIR, and throw — crashing the setup step instead of returning the friendly "path exists and is not a directory" result. A path that is a file (or missing) holds no git repo for ktx to avoid, so treat ENOTDIR like ENOENT and return 'unowned'. The downstream existingFolderState check still rejects a non-directory with its friendly message, and the classifier no longer throws raw errno for any caller.
This commit is contained in:
parent
f3f893bf01
commit
2877b85adc
20 changed files with 412 additions and 78 deletions
|
|
@ -1,6 +1,6 @@
|
|||
import { promises as fs } from 'node:fs';
|
||||
import { dirname, join } from 'node:path';
|
||||
import { CheckRepoActions, type SimpleGit } from 'simple-git';
|
||||
import type { SimpleGit } from 'simple-git';
|
||||
import { noopLogger, resolveConfigDir, type KtxCoreConfig, type KtxLogger } from './config.js';
|
||||
import { createSimpleGit } from './git-env.js';
|
||||
|
||||
|
|
@ -27,6 +27,69 @@ export interface WorktreeEntry {
|
|||
head: string | null;
|
||||
}
|
||||
|
||||
const KTX_MANAGED_GIT_CONFIG_KEY = 'ktx.managed';
|
||||
|
||||
export type KtxRepoOwnership = 'unowned' | 'ktx-managed' | 'foreign';
|
||||
|
||||
class KtxForeignGitRepositoryError extends Error {
|
||||
constructor(configDir: string) {
|
||||
super(
|
||||
`${configDir} is already a git repository that ktx did not create. ` +
|
||||
'ktx maintains its context in a repository it owns; run ktx in a dedicated directory or move the existing repository aside.',
|
||||
);
|
||||
this.name = 'KtxForeignGitRepositoryError';
|
||||
}
|
||||
}
|
||||
|
||||
function isNodeErrnoException(error: unknown): error is NodeJS.ErrnoException {
|
||||
return error instanceof Error && 'code' in error;
|
||||
}
|
||||
|
||||
/**
|
||||
* Classify whether ktx may own a git repository rooted exactly at `dir`.
|
||||
*
|
||||
* - `unowned`: there is no git repository for ktx to avoid here → ktx may
|
||||
* `git init`. Covers a fresh standalone directory, a fresh directory nested
|
||||
* inside a parent repo, a path that does not exist yet, and a path that is not
|
||||
* a directory at all (whether the path is a usable project directory is a
|
||||
* separate concern for the caller to validate).
|
||||
* - `ktx-managed`: `<dir>/.git` is a directory carrying ktx's ownership marker.
|
||||
* - `foreign`: a repo ktx did not create — a `.git` directory without the marker,
|
||||
* or a `.git` *file* (a linked worktree). ktx must never adopt or mutate it.
|
||||
*
|
||||
* Reads only `<dir>/.git` directly and never walks up the directory tree, so the
|
||||
* classification of a project dir never depends on whether a parent repo exists.
|
||||
* Shared by `GitService.initialize()` (the invariant) and the setup wizard (the
|
||||
* pre-flight guidance) so both decide ownership from the same rule.
|
||||
*/
|
||||
export async function classifyKtxRepoOwnership(dir: string): Promise<KtxRepoOwnership> {
|
||||
let dotGitIsDirectory: boolean;
|
||||
try {
|
||||
dotGitIsDirectory = (await fs.lstat(join(dir, '.git'))).isDirectory();
|
||||
} catch (error) {
|
||||
// ENOENT: `<dir>/.git` is absent. ENOTDIR: `<dir>` itself is a file, so it
|
||||
// can hold no repo. Either way there is nothing for ktx to avoid here.
|
||||
if (isNodeErrnoException(error) && (error.code === 'ENOENT' || error.code === 'ENOTDIR')) {
|
||||
return 'unowned';
|
||||
}
|
||||
throw error;
|
||||
}
|
||||
if (!dotGitIsDirectory) {
|
||||
return 'foreign';
|
||||
}
|
||||
try {
|
||||
const marker = await createSimpleGit(dir).raw([
|
||||
'config',
|
||||
'--local',
|
||||
'--get',
|
||||
KTX_MANAGED_GIT_CONFIG_KEY,
|
||||
]);
|
||||
return marker.trim() === 'true' ? 'ktx-managed' : 'foreign';
|
||||
} catch {
|
||||
return 'foreign';
|
||||
}
|
||||
}
|
||||
|
||||
export type SquashMergeResult =
|
||||
| { ok: true; squashSha: string; touchedPaths: string[] }
|
||||
| { ok: false; conflict: true; conflictPaths: string[] };
|
||||
|
|
@ -98,18 +161,17 @@ export class GitService {
|
|||
|
||||
private async initialize(): Promise<void> {
|
||||
try {
|
||||
// Adopt an existing repo ONLY when this directory is itself that repo's root.
|
||||
// When it sits below an enclosing repo, a plain checkIsRepo() is true and ktx
|
||||
// would silently piggyback on the enclosing tree — but every ktx relative path
|
||||
// (file-store writes, session worktrees, squash-merges, reindex scans) assumes
|
||||
// this directory IS the working-tree root. So treat "inside an enclosing repo"
|
||||
// the same as "no repo" and initialize a dedicated repo rooted here.
|
||||
const isRepoRoot = await this.git.checkIsRepo(CheckRepoActions.IS_REPO_ROOT);
|
||||
const ownership = await classifyKtxRepoOwnership(this.configDir);
|
||||
|
||||
if (!isRepoRoot) {
|
||||
await this.git.init();
|
||||
this.logger.log('Initialized git repository');
|
||||
if (ownership === 'foreign') {
|
||||
throw new KtxForeignGitRepositoryError(this.configDir);
|
||||
}
|
||||
if (ownership === 'unowned') {
|
||||
await this.git.init();
|
||||
await this.git.addConfig(KTX_MANAGED_GIT_CONFIG_KEY, 'true');
|
||||
this.logger.log('Initialized ktx-managed git repository');
|
||||
}
|
||||
// ownership === 'ktx-managed' → ktx's own repo; proceed with the normal re-run path.
|
||||
|
||||
// Keep any auto-maintenance triggered by writes in-process. Detached maintenance can
|
||||
// keep object-pack directories alive briefly after awaited git commands complete,
|
||||
|
|
@ -130,6 +192,11 @@ export class GitService {
|
|||
this.logger.log('Wrote bootstrap commit to config repo');
|
||||
}
|
||||
} catch (error) {
|
||||
// The foreign-repo error is already typed and actionable; surface it verbatim so every
|
||||
// command that loads the project shows the same clear guidance instead of a generic wrapper.
|
||||
if (error instanceof KtxForeignGitRepositoryError) {
|
||||
throw error;
|
||||
}
|
||||
this.logger.error('Failed to initialize git repository', error);
|
||||
// Preserve the underlying git error: the generic message alone is undiagnosable in
|
||||
// telemetry and unactionable for the user. The exception reporter walks `cause` and
|
||||
|
|
|
|||
|
|
@ -230,14 +230,13 @@ const setupSchema = z
|
|||
|
||||
const storageGitSchema = z
|
||||
.strictObject({
|
||||
auto_commit: z.boolean().default(true).describe('When true, KTX automatically commits state changes to the local Git-backed store.'),
|
||||
author: z
|
||||
.string()
|
||||
.min(1)
|
||||
.default('ktx <ktx@example.com>')
|
||||
.describe('Git author identity used for auto-commits, in standard "Name <email>" form.'),
|
||||
.describe('Git author identity used for commits, in standard "Name <email>" form.'),
|
||||
})
|
||||
.describe('Git-backed storage commit policy.');
|
||||
.describe('Git-backed storage author policy.');
|
||||
|
||||
const storageSchema = z
|
||||
.strictObject({
|
||||
|
|
@ -276,12 +275,6 @@ const agentSchema = z
|
|||
})
|
||||
.describe('Agent feature configuration.');
|
||||
|
||||
const memorySchema = z
|
||||
.strictObject({
|
||||
auto_commit: z.boolean().default(true).describe('When true, KTX automatically commits memory updates to the Git-backed store.'),
|
||||
})
|
||||
.describe('Memory subsystem configuration.');
|
||||
|
||||
const ktxProjectConfigSchema = z
|
||||
.strictObject({
|
||||
setup: setupSchema.optional().describe('Setup-wizard state. Written by `ktx setup`; may be omitted.'),
|
||||
|
|
@ -293,7 +286,6 @@ const ktxProjectConfigSchema = z
|
|||
llm: llmSchema.prefault({}).describe('LLM provider, per-role model overrides, and prompt-caching tunables.'),
|
||||
ingest: ingestSchema.prefault({}).describe('Ingest pipeline configuration.'),
|
||||
agent: agentSchema.prefault({}).describe('Agent feature configuration.'),
|
||||
memory: memorySchema.prefault({}).describe('Memory subsystem configuration.'),
|
||||
scan: scanSchema.prefault({}).describe('Schema-scan configuration: enrichment and relationship discovery.'),
|
||||
})
|
||||
.describe('Configuration schema for KTX project files (ktx.yaml).');
|
||||
|
|
|
|||
|
|
@ -66,7 +66,6 @@ function demoConfig(databasePath: string): string {
|
|||
' state: sqlite',
|
||||
' search: sqlite-fts5',
|
||||
' git:',
|
||||
' auto_commit: true',
|
||||
' author: ktx <ktx@example.com>',
|
||||
'llm:',
|
||||
' provider:',
|
||||
|
|
|
|||
|
|
@ -2,6 +2,7 @@ import { existsSync } from 'node:fs';
|
|||
import { mkdir, readdir, readFile, stat, writeFile } from 'node:fs/promises';
|
||||
import { homedir } from 'node:os';
|
||||
import { join, resolve } from 'node:path';
|
||||
import { classifyKtxRepoOwnership } from './context/core/git.service.js';
|
||||
import { initKtxProject, type KtxLocalProject, loadKtxProject } from './context/project/project.js';
|
||||
import { markKtxSetupStateStepComplete, mergeKtxSetupGitignoreEntries } from './context/project/setup-config.js';
|
||||
import { serializeKtxProjectConfig } from './context/project/config.js';
|
||||
|
|
@ -106,11 +107,32 @@ type ConfirmProjectDirResult =
|
|||
| { status: 'cancelled' }
|
||||
| { status: 'not-directory' };
|
||||
|
||||
/**
|
||||
* ktx owns the git repository at the project dir, so it refuses to create a
|
||||
* project inside a repository it did not create (which it would otherwise have
|
||||
* to adopt or fail on at first commit). Guides the user toward a dedicated
|
||||
* directory instead of letting `GitService.initialize()` throw mid-setup.
|
||||
*/
|
||||
async function ensureProjectDirIsOwnable(selectedDir: string, io: KtxCliIo): Promise<boolean> {
|
||||
if ((await classifyKtxRepoOwnership(selectedDir)) === 'foreign') {
|
||||
io.stderr.write(
|
||||
`${selectedDir} is already a git repository that ktx did not create.\n` +
|
||||
'ktx keeps its context in a repository it owns. Choose a new subfolder or an empty directory instead.\n',
|
||||
);
|
||||
return false;
|
||||
}
|
||||
return true;
|
||||
}
|
||||
|
||||
async function confirmProjectDir(
|
||||
selectedDir: string,
|
||||
io: KtxCliIo,
|
||||
prompts: KtxSetupProjectPromptAdapter,
|
||||
): Promise<ConfirmProjectDirResult> {
|
||||
if (!(await ensureProjectDirIsOwnable(selectedDir, io))) {
|
||||
return { status: 'choose-another' };
|
||||
}
|
||||
|
||||
const state = await existingFolderState(selectedDir);
|
||||
|
||||
if (state === 'not-directory') {
|
||||
|
|
@ -287,6 +309,9 @@ export async function runKtxSetupProjectStep(
|
|||
io.stderr.write('Missing setup choice: pass --yes to create a project in non-interactive setup.\n');
|
||||
return { status: 'missing-input', projectDir };
|
||||
}
|
||||
if (!(await ensureProjectDirIsOwnable(projectDir, io))) {
|
||||
return { status: 'missing-input', projectDir };
|
||||
}
|
||||
const project = await createProject(projectDir, deps);
|
||||
printProjectSummary(io, projectDir);
|
||||
return {
|
||||
|
|
@ -332,6 +357,9 @@ export async function runKtxSetupProjectStep(
|
|||
}
|
||||
|
||||
if (choice === 'current') {
|
||||
if (!(await ensureProjectDirIsOwnable(projectDir, io))) {
|
||||
continue;
|
||||
}
|
||||
const project = await createProject(projectDir, deps);
|
||||
printProjectSummary(io, projectDir);
|
||||
return {
|
||||
|
|
|
|||
|
|
@ -78,7 +78,6 @@ interface PipelineStatus {
|
|||
interface StorageStatus {
|
||||
state: string;
|
||||
search: string;
|
||||
gitAutoCommit: boolean;
|
||||
gitAuthor: string;
|
||||
}
|
||||
|
||||
|
|
@ -160,7 +159,6 @@ export interface ProjectStatus {
|
|||
nextActions: string[];
|
||||
promptCaching?: { enabled: boolean; systemTtl?: string; toolsTtl?: string; historyTtl?: string };
|
||||
workUnits?: { stepBudget: number; maxConcurrency: number; failureMode: string };
|
||||
memoryAutoCommit: boolean;
|
||||
relationshipsDetail?: {
|
||||
acceptThreshold: number;
|
||||
reviewThreshold: number;
|
||||
|
|
@ -579,7 +577,6 @@ function buildStorageStatus(config: KtxProjectConfig): StorageStatus {
|
|||
return {
|
||||
state: config.storage.state,
|
||||
search: config.storage.search,
|
||||
gitAutoCommit: config.storage.git.auto_commit,
|
||||
gitAuthor: config.storage.git.author,
|
||||
};
|
||||
}
|
||||
|
|
@ -986,7 +983,6 @@ export async function buildProjectStatus(project: KtxLocalProject, options: Buil
|
|||
maxConcurrency: config.ingest.workUnits.maxConcurrency,
|
||||
failureMode: config.ingest.workUnits.failureMode,
|
||||
},
|
||||
memoryAutoCommit: config.memory.auto_commit,
|
||||
relationshipsDetail: {
|
||||
acceptThreshold: config.scan.relationships.acceptThreshold,
|
||||
reviewThreshold: config.scan.relationships.reviewThreshold,
|
||||
|
|
@ -1272,10 +1268,7 @@ export function renderProjectStatus(status: ProjectStatus, options: RenderProjec
|
|||
lines.push(
|
||||
` ${bold('Agent')} ${dim(`max_iterations=${status.pipeline.agentMaxIterations}, tools=${status.pipeline.agentTools.join(', ') || '(none)'}`)}`,
|
||||
);
|
||||
lines.push(` ${bold('Memory')} ${dim(`auto_commit=${status.memoryAutoCommit}`)}`);
|
||||
lines.push(
|
||||
` ${bold('Git')} ${dim(`auto_commit=${status.storage.gitAutoCommit}, author=${status.storage.gitAuthor}`)}`,
|
||||
);
|
||||
lines.push(` ${bold('Git')} ${dim(`author=${status.storage.gitAuthor}`)}`);
|
||||
lines.push('');
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue