Merge remote-tracking branch 'gitea/develop' into feat/501-mcp-apikey-auth
This commit is contained in:
@@ -99,10 +99,12 @@ describe('AiSettingsService.getMasked reindex progress', () => {
|
||||
// actually pins the progress.total branch rather than coincidentally
|
||||
// matching the DB fallback. With fix #1 the two sources agree in practice,
|
||||
// but getMasked must still return progress.total when a record is active.
|
||||
const startedAt = Date.now();
|
||||
reindexProgress.get.mockResolvedValue({
|
||||
total: 500,
|
||||
done: 120,
|
||||
startedAt: Date.now(),
|
||||
startedAt,
|
||||
runId: 'run-abc',
|
||||
});
|
||||
|
||||
const masked = await service.getMasked(WORKSPACE_ID);
|
||||
@@ -110,6 +112,10 @@ describe('AiSettingsService.getMasked reindex progress', () => {
|
||||
expect(masked.indexedPages).toBe(120); // progress.done, not DB 478
|
||||
expect(masked.totalPages).toBe(500); // progress.total, not DB 478
|
||||
expect(masked.reindexing).toBe(true);
|
||||
// The status payload must carry the run identity so the client can key its
|
||||
// poll on it (a changed runId => a NEW run).
|
||||
expect(masked.runId).toBe('run-abc');
|
||||
expect(masked.reindexStartedAt).toBe(startedAt);
|
||||
});
|
||||
|
||||
it('falls back to countIndexedPages when no reindex is active', async () => {
|
||||
@@ -121,6 +127,10 @@ describe('AiSettingsService.getMasked reindex progress', () => {
|
||||
expect(masked.indexedPages).toBe(478);
|
||||
expect(masked.totalPages).toBe(478);
|
||||
expect(masked.reindexing).toBe(false);
|
||||
// No active run -> no run identity surfaced (the client keeps its prior
|
||||
// steady-state behaviour).
|
||||
expect(masked.runId).toBeUndefined();
|
||||
expect(masked.reindexStartedAt).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -245,6 +245,9 @@ export class AiSettingsService {
|
||||
// Max context window for the chat header badge denominator. Stored as
|
||||
// ::text; 0/unset/invalid = no limit (undefined).
|
||||
chatContextWindow: parsePositiveInt(provider.chatContextWindow),
|
||||
// RAW stored value (#490): the replay budgeter reads this to distinguish an
|
||||
// explicit `0` (off-switch) from unset, which parsePositiveInt cannot.
|
||||
chatContextWindowRaw: provider.chatContextWindow,
|
||||
// Plain passthrough; getChatModel defaults unset to 'openai-compatible'.
|
||||
chatApiStyle: provider.chatApiStyle,
|
||||
// Cheap model id for the anonymous public-share assistant; reuses the chat
|
||||
@@ -371,6 +374,12 @@ export class AiSettingsService {
|
||||
totalPages,
|
||||
// Optional hint for the client: a reindex run is currently in progress.
|
||||
reindexing: progress != null,
|
||||
// Per-run identity so the client can key its poll on a stable run id and
|
||||
// reset its per-run state when a NEW run starts. Present only while a run
|
||||
// is active; `runId` may be '' for a legacy/degraded record (the client
|
||||
// treats that as "no identity").
|
||||
runId: progress?.runId,
|
||||
reindexStartedAt: progress?.startedAt,
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
@@ -0,0 +1,86 @@
|
||||
// `.provider` alone cannot prove the gemini/ollama chat factories were built
|
||||
// with the instrumented streaming fetch — a regression dropping it (which drops
|
||||
// them back to the global undici fetch: no keep-alive recycle, no reset retries,
|
||||
// unbounded silence timeout; incident classes #140/#175/#310) would still pass.
|
||||
// So mock the factories and assert the exact fetch argument. jest.mock is
|
||||
// module-scoped, hence a dedicated file.
|
||||
|
||||
const mockGeminiModel = { provider: 'google.generative-ai', modelId: 'm' };
|
||||
const mockOllamaModel = { provider: 'ollama.chat', modelId: 'm' };
|
||||
|
||||
// jest allows `mock`-prefixed vars inside a jest.mock factory.
|
||||
const mockCreateGoogle = jest.fn((_settings: unknown) => () => mockGeminiModel);
|
||||
const mockCreateOllama = jest.fn((_settings: unknown) => () => mockOllamaModel);
|
||||
|
||||
jest.mock('@ai-sdk/google', () => ({
|
||||
createGoogleGenerativeAI: (settings: unknown) => mockCreateGoogle(settings),
|
||||
}));
|
||||
jest.mock('ai-sdk-ollama', () => ({
|
||||
createOllama: (settings: unknown) => mockCreateOllama(settings),
|
||||
}));
|
||||
|
||||
import { AiService } from './ai.service';
|
||||
|
||||
describe('AiService.getChatModel provider transport fetch (gemini/ollama)', () => {
|
||||
function serviceWith(cfg: Record<string, unknown>) {
|
||||
const aiSettings = {
|
||||
resolve: jest.fn().mockResolvedValue(cfg),
|
||||
};
|
||||
return new AiService(
|
||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any
|
||||
aiSettings as any,
|
||||
{ find: jest.fn() } as never,
|
||||
{ decryptSecret: jest.fn() } as never,
|
||||
);
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
mockCreateGoogle.mockClear();
|
||||
mockCreateOllama.mockClear();
|
||||
});
|
||||
|
||||
it('builds the gemini chat model with the instrumented streaming fetch', async () => {
|
||||
await serviceWith({
|
||||
driver: 'gemini',
|
||||
chatModel: 'gemini-2.5-pro',
|
||||
apiKey: 'the-key',
|
||||
}).getChatModel('ws-1');
|
||||
expect(mockCreateGoogle).toHaveBeenCalledTimes(1);
|
||||
expect(mockCreateGoogle).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
apiKey: 'the-key',
|
||||
fetch: expect.any(Function),
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it('builds the ollama chat model with the instrumented streaming fetch', async () => {
|
||||
await serviceWith({
|
||||
driver: 'ollama',
|
||||
chatModel: 'llama3',
|
||||
baseUrl: 'http://localhost:11434/api',
|
||||
}).getChatModel('ws-1');
|
||||
expect(mockCreateOllama).toHaveBeenCalledTimes(1);
|
||||
expect(mockCreateOllama).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
baseURL: 'http://localhost:11434/api',
|
||||
fetch: expect.any(Function),
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it('reuses ONE service-lifetime fetch instance across both providers', async () => {
|
||||
const svc = serviceWith({
|
||||
driver: 'gemini',
|
||||
chatModel: 'gemini-2.5-pro',
|
||||
apiKey: 'k',
|
||||
});
|
||||
await svc.getChatModel('ws-1');
|
||||
const geminiFetch = mockCreateGoogle.mock.calls[0][0] as { fetch: unknown };
|
||||
// Same instance on a second call — the fetch is held for the service
|
||||
// lifetime to reuse the streaming dispatcher's connection pool.
|
||||
await svc.getChatModel('ws-1');
|
||||
const geminiFetch2 = mockCreateGoogle.mock.calls[1][0] as { fetch: unknown };
|
||||
expect(geminiFetch.fetch).toBe(geminiFetch2.fetch);
|
||||
});
|
||||
});
|
||||
@@ -190,10 +190,22 @@ export class AiService {
|
||||
}).chat(chatModel);
|
||||
}
|
||||
case 'gemini':
|
||||
return createGoogleGenerativeAI({ apiKey })(chatModel);
|
||||
// Route gemini through the same instrumented streaming fetch as openai
|
||||
// (finite silence timeouts + keep-alive recycling + pre-response
|
||||
// connection-reset retry). Without it the provider ran on the global
|
||||
// undici fetch — no keep-alive recycle, no reset retries, default
|
||||
// (unbounded silence) timeout — so incident classes #140/#175/#310 were
|
||||
// reproducible for gemini too.
|
||||
return createGoogleGenerativeAI({
|
||||
apiKey,
|
||||
fetch: this.aiProviderFetch,
|
||||
})(chatModel);
|
||||
case 'ollama':
|
||||
// Ollama needs no API key.
|
||||
return createOllama({ baseURL: baseUrl })(chatModel);
|
||||
// Ollama needs no API key. Same transport hardening as above (#140/#175/#310).
|
||||
return createOllama({
|
||||
baseURL: baseUrl,
|
||||
fetch: this.aiProviderFetch,
|
||||
})(chatModel);
|
||||
default:
|
||||
throw new AiNotConfiguredException();
|
||||
}
|
||||
|
||||
@@ -105,6 +105,10 @@ export interface ResolvedAiConfig extends Partial<AiProviderSettings> {
|
||||
// Max context window in tokens; surfaced to the chat header badge as the
|
||||
// "current / max" denominator. 0/unset = no limit.
|
||||
chatContextWindow?: number;
|
||||
// RAW stored context window (::text), BEFORE parsePositiveInt collapses `0` and
|
||||
// unset to `undefined`. The #490 replay budgeter needs the raw value to honor an
|
||||
// explicit `0` off-switch distinctly from "unset -> flat default".
|
||||
chatContextWindowRaw?: string | number;
|
||||
// Cheap model id for the public-share assistant; reuses the chat creds.
|
||||
publicShareChatModel?: string;
|
||||
// Agent-role id whose persona the public-share assistant adopts (empty/unset
|
||||
@@ -149,4 +153,14 @@ export interface MaskedAiSettings {
|
||||
// True while a full workspace reindex is actively running (the counts above
|
||||
// then reflect the live run progress rather than the steady-state DB count).
|
||||
reindexing?: boolean;
|
||||
// Identity of the ACTIVE reindex run (present only while `reindexing`). The
|
||||
// client keys its poll on `runId`: a changed value means a NEW run (reset the
|
||||
// per-run poll state it latched), the same value means the run it is already
|
||||
// watching — removing the "same run or a fresh one?" ambiguity a stale
|
||||
// pre-reindex snapshot otherwise causes. Absent/empty degrades gracefully.
|
||||
runId?: string;
|
||||
// Epoch-ms the active run started (present only while `reindexing`). Paired
|
||||
// with `runId` so a run that restarts with the same (recycled) id is still
|
||||
// seen as new.
|
||||
reindexStartedAt?: number;
|
||||
}
|
||||
|
||||
@@ -48,19 +48,38 @@ describe('EmbeddingReindexProgressService', () => {
|
||||
}
|
||||
|
||||
describe('get', () => {
|
||||
it('maps a valid hash to a ReindexProgress object', async () => {
|
||||
it('maps a valid hash to a ReindexProgress object (incl. the run identity)', async () => {
|
||||
const { redis, hgetall } = makeRedis();
|
||||
hgetall.mockResolvedValue({ total: '478', done: '120', startedAt: '1000' });
|
||||
hgetall.mockResolvedValue({
|
||||
total: '478',
|
||||
done: '120',
|
||||
startedAt: '1000',
|
||||
runId: 'run-xyz',
|
||||
});
|
||||
const service = makeService(redis);
|
||||
|
||||
await expect(service.get(WORKSPACE_ID)).resolves.toEqual({
|
||||
total: 478,
|
||||
done: 120,
|
||||
startedAt: 1000,
|
||||
runId: 'run-xyz',
|
||||
});
|
||||
expect(hgetall).toHaveBeenCalledWith(KEY);
|
||||
});
|
||||
|
||||
it('degrades a missing runId to an empty string (legacy/partial record)', async () => {
|
||||
const { redis, hgetall } = makeRedis();
|
||||
// A record written before runId existed: get() must still succeed and
|
||||
// report runId='' so the client treats it as "no identity", never breaks.
|
||||
hgetall.mockResolvedValue({ total: '10', done: '3', startedAt: '5' });
|
||||
await expect(makeService(redis).get(WORKSPACE_ID)).resolves.toEqual({
|
||||
total: 10,
|
||||
done: 3,
|
||||
startedAt: 5,
|
||||
runId: '',
|
||||
});
|
||||
});
|
||||
|
||||
it('returns null for an empty hash (no record)', async () => {
|
||||
const { redis, hgetall } = makeRedis();
|
||||
hgetall.mockResolvedValue({});
|
||||
@@ -87,11 +106,17 @@ describe('EmbeddingReindexProgressService', () => {
|
||||
|
||||
it('coerces a non-finite startedAt to 0', async () => {
|
||||
const { redis, hgetall } = makeRedis();
|
||||
hgetall.mockResolvedValue({ total: '10', done: '2', startedAt: 'nope' });
|
||||
hgetall.mockResolvedValue({
|
||||
total: '10',
|
||||
done: '2',
|
||||
startedAt: 'nope',
|
||||
runId: 'run-1',
|
||||
});
|
||||
await expect(makeService(redis).get(WORKSPACE_ID)).resolves.toEqual({
|
||||
total: 10,
|
||||
done: 2,
|
||||
startedAt: 0,
|
||||
runId: 'run-1',
|
||||
});
|
||||
});
|
||||
|
||||
@@ -115,6 +140,21 @@ describe('EmbeddingReindexProgressService', () => {
|
||||
expect(multiObj.exec).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('mints a fresh non-empty runId into the record on each start', async () => {
|
||||
const { redis, multiObj } = makeRedis();
|
||||
const service = makeService(redis);
|
||||
await service.start(WORKSPACE_ID, 1);
|
||||
await service.start(WORKSPACE_ID, 1);
|
||||
|
||||
const firstRunId = multiObj.hset.mock.calls[0][1].runId;
|
||||
const secondRunId = multiObj.hset.mock.calls[1][1].runId;
|
||||
expect(typeof firstRunId).toBe('string');
|
||||
expect(firstRunId).not.toBe('');
|
||||
// Each run gets its OWN identity so the client can tell a re-trigger apart
|
||||
// from the run it is already watching.
|
||||
expect(secondRunId).not.toBe(firstRunId);
|
||||
});
|
||||
|
||||
it('defaults the expire TTL to the full 1h record TTL', async () => {
|
||||
const { redis, multiObj } = makeRedis();
|
||||
await makeService(redis).start(WORKSPACE_ID, 478);
|
||||
|
||||
@@ -1,17 +1,28 @@
|
||||
import { Injectable, Logger } from '@nestjs/common';
|
||||
import { RedisService } from '@nestjs-labs/nestjs-ioredis';
|
||||
import { randomUUID } from 'node:crypto';
|
||||
import type { Redis } from 'ioredis';
|
||||
|
||||
/**
|
||||
* Live progress of an in-flight workspace embeddings reindex run.
|
||||
* `total` is the number of pages the run will process, `done` how many it has
|
||||
* already processed (success OR handled failure), `startedAt` the epoch-ms the
|
||||
* record was created.
|
||||
* record was created, and `runId` a per-run identity minted at `start()`.
|
||||
*
|
||||
* `runId` gives each reindex run a stable identity so a poller can tell "same
|
||||
* run I've been watching" from "a NEW run started" WITHOUT guessing from the
|
||||
* progress counters (the ambiguity that a stale pre-reindex snapshot vs a fresh
|
||||
* run otherwise causes — the bug class fixed twice under #262). It is best-
|
||||
* effort like the rest of this record: a record written before this field
|
||||
* existed (or a Redis hiccup) yields an empty `runId`, which the client must
|
||||
* treat as "no identity available" and degrade to its prior behaviour, never
|
||||
* break.
|
||||
*/
|
||||
export interface ReindexProgress {
|
||||
total: number;
|
||||
done: number;
|
||||
startedAt: number;
|
||||
runId: string;
|
||||
}
|
||||
|
||||
/** Redis key namespace for the per-workspace reindex-progress record. */
|
||||
@@ -86,12 +97,18 @@ export class EmbeddingReindexProgressService {
|
||||
): Promise<void> {
|
||||
const key = this.key(workspaceId);
|
||||
try {
|
||||
// A fresh identity per run so the client poll can key on it: a changed
|
||||
// runId means a genuinely NEW run (reset any latched per-run poll state),
|
||||
// the same runId means the run the client is already watching. Best-effort
|
||||
// like the counters — never surfaced to the user, only used to disambiguate.
|
||||
const runId = randomUUID();
|
||||
await this.redis
|
||||
.multi()
|
||||
.hset(key, {
|
||||
total: String(total),
|
||||
done: '0',
|
||||
startedAt: String(Date.now()),
|
||||
runId,
|
||||
})
|
||||
.expire(key, ttlSeconds)
|
||||
.exec();
|
||||
@@ -150,7 +167,15 @@ export class EmbeddingReindexProgressService {
|
||||
const done = Number(data.done);
|
||||
const startedAt = Number(data.startedAt);
|
||||
if (!Number.isFinite(total) || !Number.isFinite(done)) return null;
|
||||
return { total, done, startedAt: Number.isFinite(startedAt) ? startedAt : 0 };
|
||||
// `runId` degrades gracefully: a pre-existing record (written before this
|
||||
// field) or a stripped value reads as '' — the client treats that as "no
|
||||
// identity" and keeps its prior behaviour rather than breaking the poll.
|
||||
return {
|
||||
total,
|
||||
done,
|
||||
startedAt: Number.isFinite(startedAt) ? startedAt : 0,
|
||||
runId: typeof data.runId === 'string' ? data.runId : '',
|
||||
};
|
||||
} catch (err) {
|
||||
this.logger.warn(
|
||||
`reindex-progress read failed for workspace ${workspaceId}; ` +
|
||||
|
||||
@@ -1,14 +1,19 @@
|
||||
import { Global, Module } from '@nestjs/common';
|
||||
import { AUDIT_SERVICE, NoopAuditService } from './audit.service';
|
||||
import { AUDIT_SERVICE } from './audit.service';
|
||||
import { DatabaseAuditService } from './database-audit.service';
|
||||
|
||||
// #496: bind the audit token to a real DB-backed trail (was NoopAuditService,
|
||||
// which silently dropped every event). Kysely (@Global DatabaseModule) and
|
||||
// ClsService (@Global ClsModule) are both globally available, so this module
|
||||
// needs no extra imports.
|
||||
@Global()
|
||||
@Module({
|
||||
providers: [
|
||||
{
|
||||
provide: AUDIT_SERVICE,
|
||||
useClass: NoopAuditService,
|
||||
useClass: DatabaseAuditService,
|
||||
},
|
||||
],
|
||||
exports: [AUDIT_SERVICE],
|
||||
})
|
||||
export class NoopAuditModule {}
|
||||
export class AuditModule {}
|
||||
|
||||
@@ -0,0 +1,198 @@
|
||||
import { Logger } from '@nestjs/common';
|
||||
import { DatabaseAuditService } from './database-audit.service';
|
||||
import { AUDIT_CONTEXT_KEY } from '../../common/middlewares/audit-context.middleware';
|
||||
import { AuditEvent, AuditResource } from '../../common/events/audit-events';
|
||||
|
||||
/**
|
||||
* Observable-property coverage for the DB-backed audit trail (#496): every
|
||||
* assertion pins what actually reaches the `audit` table (or that nothing does),
|
||||
* driven through a chainable Kysely mock that captures the inserted rows.
|
||||
*/
|
||||
describe('DatabaseAuditService', () => {
|
||||
function makeService(clsContext: any) {
|
||||
const inserted: any[] = [];
|
||||
const updated: any[] = [];
|
||||
let failNextInsert = false;
|
||||
|
||||
const db: any = {
|
||||
insertInto: jest.fn(() => ({
|
||||
values: jest.fn((rows: any) => ({
|
||||
execute: jest.fn(async () => {
|
||||
if (failNextInsert) {
|
||||
failNextInsert = false;
|
||||
throw new Error('boom');
|
||||
}
|
||||
inserted.push(rows);
|
||||
}),
|
||||
})),
|
||||
})),
|
||||
updateTable: jest.fn(() => ({
|
||||
set: jest.fn((patch: any) => ({
|
||||
where: jest.fn(() => ({
|
||||
execute: jest.fn(async () => {
|
||||
updated.push(patch);
|
||||
}),
|
||||
})),
|
||||
})),
|
||||
})),
|
||||
};
|
||||
|
||||
const store: Record<string, any> = { [AUDIT_CONTEXT_KEY]: clsContext };
|
||||
const cls: any = {
|
||||
get: jest.fn((key: string) => store[key]),
|
||||
set: jest.fn((key: string, val: any) => {
|
||||
store[key] = val;
|
||||
}),
|
||||
};
|
||||
|
||||
const service = new DatabaseAuditService(db, cls);
|
||||
return {
|
||||
service,
|
||||
inserted,
|
||||
updated,
|
||||
cls,
|
||||
store,
|
||||
failInsert: () => {
|
||||
failNextInsert = true;
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
const applyPayload = () => ({
|
||||
event: AuditEvent.COMMENT_SUGGESTION_APPLIED,
|
||||
resourceType: AuditResource.COMMENT,
|
||||
resourceId: 'c-1',
|
||||
spaceId: 'space-1',
|
||||
metadata: { pageId: 'p-1', suggestedText: 'new', decidedBy: 'u-2' },
|
||||
});
|
||||
|
||||
const ctx = (over?: any) => ({
|
||||
workspaceId: 'ws-1',
|
||||
actorId: 'u-2',
|
||||
actorType: 'user',
|
||||
ipAddress: '10.0.0.1',
|
||||
userAgent: 'jest',
|
||||
...over,
|
||||
});
|
||||
|
||||
it('log() persists a row with the CLS context merged onto the payload', async () => {
|
||||
const { service, inserted } = makeService(ctx());
|
||||
service.log(applyPayload());
|
||||
// log() is fire-and-forget; flush the microtask queue.
|
||||
await Promise.resolve();
|
||||
await Promise.resolve();
|
||||
|
||||
expect(inserted).toHaveLength(1);
|
||||
expect(inserted[0]).toMatchObject({
|
||||
workspaceId: 'ws-1',
|
||||
actorId: 'u-2',
|
||||
actorType: 'user',
|
||||
event: AuditEvent.COMMENT_SUGGESTION_APPLIED,
|
||||
resourceType: AuditResource.COMMENT,
|
||||
resourceId: 'c-1',
|
||||
spaceId: 'space-1',
|
||||
ipAddress: '10.0.0.1',
|
||||
metadata: { pageId: 'p-1', suggestedText: 'new', decidedBy: 'u-2' },
|
||||
});
|
||||
});
|
||||
|
||||
it('log() is a no-op when there is no workspace in scope', async () => {
|
||||
const { service, inserted } = makeService(ctx({ workspaceId: null }));
|
||||
service.log(applyPayload());
|
||||
await Promise.resolve();
|
||||
expect(inserted).toHaveLength(0);
|
||||
});
|
||||
|
||||
it('log() drops EXCLUDED_AUDIT_EVENTS (e.g. comment.created)', async () => {
|
||||
const { service, inserted } = makeService(ctx());
|
||||
service.log({
|
||||
event: AuditEvent.COMMENT_CREATED,
|
||||
resourceType: AuditResource.COMMENT,
|
||||
resourceId: 'c-1',
|
||||
spaceId: 'space-1',
|
||||
});
|
||||
await Promise.resolve();
|
||||
await Promise.resolve();
|
||||
expect(inserted).toHaveLength(0);
|
||||
});
|
||||
|
||||
it('a failed insert is swallowed with a warn and floats no rejection (audit is a side-record)', async () => {
|
||||
// This pins the load-bearing swallow inside persist(). Because log() is
|
||||
// fire-and-forget (`void this.persist(...)`), it always returns synchronously
|
||||
// without throwing — so `not.toThrow()` alone would stay green even if the
|
||||
// try/catch were removed. We instead observe the two effects the catch is
|
||||
// responsible for: a warn IS emitted, and NO unhandled rejection floats.
|
||||
// Removing persist()'s try/catch reddens both assertions (warn count 0 + a
|
||||
// captured rejection).
|
||||
const warnSpy = jest
|
||||
.spyOn(Logger.prototype, 'warn')
|
||||
.mockImplementation(() => undefined as any);
|
||||
const rejections: unknown[] = [];
|
||||
const onRejection = (err: unknown) => rejections.push(err);
|
||||
process.on('unhandledRejection', onRejection);
|
||||
try {
|
||||
const { service, failInsert } = makeService(ctx());
|
||||
failInsert();
|
||||
expect(() => service.log(applyPayload())).not.toThrow();
|
||||
// Flush microtasks so the rejected insert settles, then give any floated
|
||||
// rejection a macrotask tick to be reported by the runtime.
|
||||
await Promise.resolve();
|
||||
await Promise.resolve();
|
||||
await new Promise((r) => setImmediate(r));
|
||||
|
||||
expect(warnSpy).toHaveBeenCalledTimes(1);
|
||||
expect(String(warnSpy.mock.calls[0][0])).toContain(
|
||||
'Failed to persist audit event',
|
||||
);
|
||||
expect(rejections).toHaveLength(0);
|
||||
} finally {
|
||||
process.off('unhandledRejection', onRejection);
|
||||
warnSpy.mockRestore();
|
||||
}
|
||||
});
|
||||
|
||||
it('logWithContext() persists with an explicit (non-request) context', async () => {
|
||||
const { service, inserted } = makeService(undefined);
|
||||
service.logWithContext(applyPayload(), ctx({ actorType: 'system' }) as any);
|
||||
await Promise.resolve();
|
||||
await Promise.resolve();
|
||||
expect(inserted).toHaveLength(1);
|
||||
expect(inserted[0].actorType).toBe('system');
|
||||
expect(inserted[0].workspaceId).toBe('ws-1');
|
||||
});
|
||||
|
||||
it('logBatchWithContext() inserts only non-excluded events', async () => {
|
||||
const { service, inserted } = makeService(undefined);
|
||||
service.logBatchWithContext(
|
||||
[
|
||||
applyPayload(),
|
||||
{
|
||||
event: AuditEvent.COMMENT_CREATED,
|
||||
resourceType: AuditResource.COMMENT,
|
||||
resourceId: 'c-2',
|
||||
},
|
||||
],
|
||||
ctx() as any,
|
||||
);
|
||||
await Promise.resolve();
|
||||
await Promise.resolve();
|
||||
// Batch is a single insert call carrying only the applied event.
|
||||
expect(inserted).toHaveLength(1);
|
||||
expect(inserted[0]).toHaveLength(1);
|
||||
expect(inserted[0][0].event).toBe(AuditEvent.COMMENT_SUGGESTION_APPLIED);
|
||||
});
|
||||
|
||||
it('setActorId / setActorType mutate the ambient CLS context', () => {
|
||||
const { service, store } = makeService(ctx({ actorId: null }));
|
||||
service.setActorId('u-9');
|
||||
service.setActorType('api_key');
|
||||
expect(store[AUDIT_CONTEXT_KEY].actorId).toBe('u-9');
|
||||
expect(store[AUDIT_CONTEXT_KEY].actorType).toBe('api_key');
|
||||
});
|
||||
|
||||
it('updateRetention() writes the workspace retention window', async () => {
|
||||
const { service, updated } = makeService(ctx());
|
||||
await service.updateRetention('ws-1', 30);
|
||||
expect(updated).toEqual([{ auditRetentionDays: 30 }]);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,160 @@
|
||||
import { Injectable, Logger } from '@nestjs/common';
|
||||
import { InjectKysely } from 'nestjs-kysely';
|
||||
import { ClsService } from 'nestjs-cls';
|
||||
import { KyselyDB } from '@docmost/db/types/kysely.types';
|
||||
import {
|
||||
AuditContext,
|
||||
AUDIT_CONTEXT_KEY,
|
||||
} from '../../common/middlewares/audit-context.middleware';
|
||||
import {
|
||||
AuditLogPayload,
|
||||
ActorType,
|
||||
EXCLUDED_AUDIT_EVENTS,
|
||||
} from '../../common/events/audit-events';
|
||||
import { AuditLogContext, IAuditService } from './audit.service';
|
||||
|
||||
/**
|
||||
* Minimal DB-backed audit trail (#496). Replaces NoopAuditService so that
|
||||
* decision-bearing events — notably comment.suggestion_applied /
|
||||
* comment.suggestion_dismissed, whose subject comment is HARD-DELETED on the
|
||||
* childless path — leave a durable record of who decided what. Without this the
|
||||
* events were emitted (comment.service / *.controller) but swallowed, so an
|
||||
* applied/dismissed suggestion was unrecoverable once the row was gone.
|
||||
*
|
||||
* Rows land in the pre-existing `audit` table (migration 20260228T223532). The
|
||||
* per-request actor/workspace/ip come from the CLS AuditContext populated by
|
||||
* AuditContextMiddleware + AuditActorInterceptor; callers that run OUTSIDE a
|
||||
* request (queue workers, imports) pass an explicit context via
|
||||
* logWithContext / logBatchWithContext.
|
||||
*
|
||||
* Audit is a side-record: a write failure MUST NOT break the originating
|
||||
* request, so every persistence path swallows its error with a warn. Events in
|
||||
* EXCLUDED_AUDIT_EVENTS (high-volume/low-signal) are dropped.
|
||||
*/
|
||||
@Injectable()
|
||||
export class DatabaseAuditService implements IAuditService {
|
||||
private readonly logger = new Logger(DatabaseAuditService.name);
|
||||
|
||||
constructor(
|
||||
@InjectKysely() private readonly db: KyselyDB,
|
||||
private readonly cls: ClsService,
|
||||
) {}
|
||||
|
||||
/**
|
||||
* Persist a single event using the ambient request-scoped AuditContext. A
|
||||
* no-op when there is no workspace in scope (the table's workspace_id is NOT
|
||||
* NULL) or the event is excluded. Fire-and-forget: the returned promise is not
|
||||
* awaited by hot callers, and its rejection is swallowed here.
|
||||
*/
|
||||
log(payload: AuditLogPayload): void {
|
||||
const context = this.cls?.get<AuditContext>(AUDIT_CONTEXT_KEY);
|
||||
if (!context?.workspaceId) {
|
||||
// No workspace in scope — nothing we can attribute the row to. This is
|
||||
// expected for events emitted outside an HTTP request; those callers must
|
||||
// use logWithContext instead.
|
||||
return;
|
||||
}
|
||||
void this.persist(payload, {
|
||||
workspaceId: context.workspaceId,
|
||||
actorId: context.actorId ?? undefined,
|
||||
actorType: context.actorType,
|
||||
ipAddress: context.ipAddress ?? undefined,
|
||||
userAgent: context.userAgent ?? undefined,
|
||||
});
|
||||
}
|
||||
|
||||
/** Persist a single event with an explicit (non-request) context. */
|
||||
logWithContext(payload: AuditLogPayload, context: AuditLogContext): void {
|
||||
if (!context?.workspaceId) return;
|
||||
void this.persist(payload, context);
|
||||
}
|
||||
|
||||
/** Persist a batch of events sharing one explicit context (imports). */
|
||||
logBatchWithContext(
|
||||
payloads: AuditLogPayload[],
|
||||
context: AuditLogContext,
|
||||
): void {
|
||||
if (!context?.workspaceId || payloads.length === 0) return;
|
||||
const rows = payloads
|
||||
.filter((p) => !EXCLUDED_AUDIT_EVENTS.has(p.event))
|
||||
.map((p) => this.toRow(p, context));
|
||||
if (rows.length === 0) return;
|
||||
this.db
|
||||
.insertInto('audit')
|
||||
.values(rows)
|
||||
.execute()
|
||||
.catch((err: any) =>
|
||||
this.logger.warn(`Failed to persist ${rows.length} audit events: ${err?.message}`),
|
||||
);
|
||||
}
|
||||
|
||||
/** Update the ambient request actor (e.g. after login resolves the user). */
|
||||
setActorId(actorId: string): void {
|
||||
const context = this.cls?.get<AuditContext>(AUDIT_CONTEXT_KEY);
|
||||
if (context) {
|
||||
context.actorId = actorId;
|
||||
this.cls.set(AUDIT_CONTEXT_KEY, context);
|
||||
}
|
||||
}
|
||||
|
||||
/** Update the ambient request actor type (user | system | api_key). */
|
||||
setActorType(actorType: ActorType): void {
|
||||
const context = this.cls?.get<AuditContext>(AUDIT_CONTEXT_KEY);
|
||||
if (context) {
|
||||
context.actorType = actorType;
|
||||
this.cls.set(AUDIT_CONTEXT_KEY, context);
|
||||
}
|
||||
}
|
||||
|
||||
/** Persist a workspace's audit-log retention window (days). */
|
||||
async updateRetention(
|
||||
workspaceId: string,
|
||||
retentionDays: number,
|
||||
): Promise<void> {
|
||||
try {
|
||||
await this.db
|
||||
.updateTable('workspaces')
|
||||
.set({ auditRetentionDays: retentionDays })
|
||||
.where('id', '=', workspaceId)
|
||||
.execute();
|
||||
} catch (err: any) {
|
||||
this.logger.warn(
|
||||
`Failed to update audit retention for workspace ${workspaceId}: ${err?.message}`,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
private async persist(
|
||||
payload: AuditLogPayload,
|
||||
context: AuditLogContext,
|
||||
): Promise<void> {
|
||||
if (EXCLUDED_AUDIT_EVENTS.has(payload.event)) return;
|
||||
try {
|
||||
await this.db
|
||||
.insertInto('audit')
|
||||
.values(this.toRow(payload, context))
|
||||
.execute();
|
||||
} catch (err: any) {
|
||||
// Audit is a side-record; never let a failed write surface to the caller.
|
||||
this.logger.warn(
|
||||
`Failed to persist audit event ${payload.event}: ${err?.message}`,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
private toRow(payload: AuditLogPayload, context: AuditLogContext) {
|
||||
return {
|
||||
workspaceId: context.workspaceId,
|
||||
actorId: context.actorId ?? null,
|
||||
actorType: context.actorType ?? 'user',
|
||||
event: payload.event,
|
||||
resourceType: payload.resourceType,
|
||||
resourceId: payload.resourceId ?? null,
|
||||
spaceId: payload.spaceId ?? null,
|
||||
// jsonb columns: node-postgres serializes plain objects to JSON.
|
||||
changes: payload.changes ? (payload.changes as any) : null,
|
||||
metadata: payload.metadata ? (payload.metadata as any) : null,
|
||||
ipAddress: context.ipAddress ?? null,
|
||||
};
|
||||
}
|
||||
}
|
||||
@@ -22,10 +22,12 @@ import { v7 } from 'uuid';
|
||||
import { generateJitteredKeyBetween } from 'fractional-indexing-jittered';
|
||||
import { FileTask, InsertablePage } from '@docmost/db/types/entity.types';
|
||||
import { canonicalizeFootnotes } from '@docmost/editor-ext';
|
||||
import { markdownToProseMirror } from '@docmost/prosemirror-markdown';
|
||||
import {
|
||||
markdownToProseMirror,
|
||||
normalizeForeignMarkdown,
|
||||
} from '@docmost/prosemirror-markdown';
|
||||
import { getProsemirrorContent } from '../../../common/helpers/prosemirror/utils';
|
||||
import { formatImportHtml } from '../utils/import-formatter';
|
||||
import { normalizeForeignMarkdown } from '../utils/foreign-markdown';
|
||||
import {
|
||||
buildAttachmentCandidates,
|
||||
collectMarkdownAndHtmlFiles,
|
||||
|
||||
@@ -18,8 +18,10 @@ import { generateJitteredKeyBetween } from 'fractional-indexing-jittered';
|
||||
import { TiptapTransformer } from '@hocuspocus/transformer';
|
||||
import * as Y from 'yjs';
|
||||
import { canonicalizeFootnotes } from '@docmost/editor-ext';
|
||||
import { markdownToProseMirror } from '@docmost/prosemirror-markdown';
|
||||
import { normalizeForeignMarkdown } from '../utils/foreign-markdown';
|
||||
import {
|
||||
markdownToProseMirror,
|
||||
normalizeForeignMarkdown,
|
||||
} from '@docmost/prosemirror-markdown';
|
||||
import {
|
||||
FileTaskStatus,
|
||||
FileTaskType,
|
||||
|
||||
@@ -1,218 +0,0 @@
|
||||
import {
|
||||
convertProseMirrorToMarkdown,
|
||||
markdownToProseMirror,
|
||||
} from '@docmost/prosemirror-markdown';
|
||||
import { normalizeForeignMarkdown } from './foreign-markdown';
|
||||
|
||||
/**
|
||||
* STEP 2 goldens for issue #345: the foreign-markdown normalizer that runs at the
|
||||
* import boundary BEFORE the strict canonical parser (`markdownToProseMirror`).
|
||||
*
|
||||
* Two layers:
|
||||
* 1. PURE string→string cases pinning the normalizer's own behavior (GFM
|
||||
* reference footnotes → inline `^[…]`).
|
||||
* 2. END-TO-END acceptance: for a foreign corpus, `normalizeForeignMarkdown`
|
||||
* then `markdownToProseMirror` then `convertProseMirrorToMarkdown` must leave
|
||||
* NO literal `[^id]` / `:::` garbage in the document and must re-export in the
|
||||
* canonical forms.
|
||||
*/
|
||||
|
||||
describe('normalizeForeignMarkdown — GFM reference footnotes', () => {
|
||||
it('inlines a single-line reference footnote and drops its definition', () => {
|
||||
const out = normalizeForeignMarkdown(
|
||||
'A note[^1] here.\n\n[^1]: The definition.',
|
||||
);
|
||||
expect(out).toBe('A note^[The definition.] here.\n');
|
||||
});
|
||||
|
||||
it('inlines every reference to a reused id (downstream dedups)', () => {
|
||||
const out = normalizeForeignMarkdown(
|
||||
'X[^a] and Y[^a].\n\n[^a]: shared.',
|
||||
);
|
||||
expect(out).toBe('X^[shared.] and Y^[shared.].\n');
|
||||
});
|
||||
|
||||
it('joins indented continuation lines of a definition with a space', () => {
|
||||
const out = normalizeForeignMarkdown(
|
||||
'See[^n].\n\n[^n]: line one\n line two',
|
||||
);
|
||||
expect(out).toBe('See^[line one line two].\n');
|
||||
});
|
||||
|
||||
it('never rewrites a reference inside a fenced code block', () => {
|
||||
const out = normalizeForeignMarkdown(
|
||||
'```\ncode[^1] here\n```\n\n[^1]: def.',
|
||||
);
|
||||
expect(out).toContain('code[^1] here');
|
||||
// The (now orphaned) definition line is still removed.
|
||||
expect(out).not.toContain('[^1]: def.');
|
||||
});
|
||||
|
||||
it('never rewrites a reference inside an INLINE-code span (backticks)', () => {
|
||||
// The `[^1]` inside backticks is literal code and must survive verbatim;
|
||||
// the one outside is rewritten. (Bug #1: only fenced blocks were protected.)
|
||||
const out = normalizeForeignMarkdown(
|
||||
'Use `arr[^1]` in code but note[^1] in prose.\n\n[^1]: def.',
|
||||
);
|
||||
expect(out).toBe('Use `arr[^1]` in code but note^[def.] in prose.\n');
|
||||
});
|
||||
|
||||
it('escapes brackets in a body so an unbalanced ] cannot truncate the footnote', () => {
|
||||
// A foreign definition body with a stray `]` would, unescaped, close the
|
||||
// canonical `^[...]` early and leak the tail as text (bug #2). The body's
|
||||
// brackets are backslash-escaped so the footnote stays whole.
|
||||
const out = normalizeForeignMarkdown(
|
||||
'Ref[^1] here.\n\n[^1]: see item ] and [more] later',
|
||||
);
|
||||
expect(out).toBe('Ref^[see item \\] and \\[more\\] later] here.\n');
|
||||
// The tokenizer must see exactly one unescaped closing bracket (our own).
|
||||
expect(out.match(/(?<!\\)\]/g)).toHaveLength(1);
|
||||
});
|
||||
|
||||
it('leaves a reference with no matching definition literal (no body to inline)', () => {
|
||||
const out = normalizeForeignMarkdown('Dangling[^x] ref.');
|
||||
expect(out).toBe('Dangling[^x] ref.');
|
||||
});
|
||||
|
||||
it('returns the input unchanged when there are no reference footnotes', () => {
|
||||
const md = '# Title\n\nJust text with `inline code` and a [link](/x).';
|
||||
expect(normalizeForeignMarkdown(md)).toBe(md);
|
||||
});
|
||||
|
||||
it('does NOT touch callout surfaces — the canonical parser handles them', () => {
|
||||
const callouts = ':::info\nHi\n:::\n\n> [!warning]\n> Careful';
|
||||
expect(normalizeForeignMarkdown(callouts)).toBe(callouts);
|
||||
});
|
||||
|
||||
it('strips a leading YAML front-matter block (Obsidian/Hugo/git-sync files)', () => {
|
||||
const out = normalizeForeignMarkdown(
|
||||
'---\ntitle: My Page\ntags: [a, b]\n---\n\n# Heading\n\nBody.',
|
||||
);
|
||||
expect(out).toBe('# Heading\n\nBody.');
|
||||
// The front-matter must not leak into the body as a setext heading.
|
||||
expect(out).not.toContain('title: My Page');
|
||||
expect(out).not.toContain('---');
|
||||
});
|
||||
|
||||
it('does not strip a horizontal rule that is not leading front-matter', () => {
|
||||
const md = 'Intro paragraph.\n\n---\n\nAfter the rule.';
|
||||
expect(normalizeForeignMarkdown(md)).toBe(md);
|
||||
});
|
||||
|
||||
it('is linear on a document with thousands of definitions (no quadratic blowup)', () => {
|
||||
// F2(a): the pass-2 rewrite must be O(text), not O(text × defs). Build a
|
||||
// pathological doc (many defs + many plain text lines) and assert it
|
||||
// completes well under a second — a quadratic implementation took ~14s.
|
||||
const N = 4000;
|
||||
const refs = Array.from({ length: N }, (_, i) => `line ${i} plain text`).join('\n');
|
||||
const defs = Array.from({ length: N }, (_, i) => `[^n${i}]: def ${i}`).join('\n');
|
||||
const doc = `start[^n0] and[^n${N - 1}] end\n\n${refs}\n\n${defs}`;
|
||||
const t0 = Date.now();
|
||||
const out = normalizeForeignMarkdown(doc);
|
||||
const elapsed = Date.now() - t0;
|
||||
expect(elapsed).toBeLessThan(2000);
|
||||
// Sanity: the two real references were still inlined.
|
||||
expect(out).toContain('^[def 0]');
|
||||
expect(out).toContain(`^[def ${N - 1}]`);
|
||||
});
|
||||
|
||||
it('is bounded on a long unclosed backtick run (no inline-split ReDoS)', () => {
|
||||
// F2(b): a huge unterminated backtick run must not cause quadratic
|
||||
// backtracking in the inline-code split. Oversized lines skip the split
|
||||
// entirely (left untouched), so this returns promptly.
|
||||
const line = 'x' + '`'.repeat(200000);
|
||||
const doc = `${line}\n\n[^1]: def`;
|
||||
const t0 = Date.now();
|
||||
normalizeForeignMarkdown(doc);
|
||||
expect(Date.now() - t0).toBeLessThan(2000);
|
||||
});
|
||||
|
||||
it('does not crash or slow down on thousands of prefix-chain definition ids', () => {
|
||||
// F7: the rewrite must use a FIXED generic scanner, not an alternation built
|
||||
// from the ids. A `(a|aa|aaa|…)` alternation over prefix-chain ids blows the
|
||||
// V8 regex compiler (FATAL RegExpCompiler Allocation failed — uncatchable,
|
||||
// kills the process). A fixed scanner has no id-dependent compilation cost.
|
||||
const N = 4000;
|
||||
const ids = Array.from({ length: N }, (_, i) => 'a'.repeat(i + 1));
|
||||
const defs = ids.map((id) => `[^${id}]: body ${id.length}`).join('\n');
|
||||
const doc = `ref[^${ids[0]}] and[^${ids[N - 1]}] end\n\n${defs}`;
|
||||
const t0 = Date.now();
|
||||
const out = normalizeForeignMarkdown(doc);
|
||||
expect(Date.now() - t0).toBeLessThan(2000);
|
||||
// Prefix disambiguation is correct: [^a] and [^aaaa...] inline their OWN body.
|
||||
expect(out).toContain('^[body 1]');
|
||||
expect(out).toContain(`^[body ${N}]`);
|
||||
});
|
||||
|
||||
it('strips a CRLF (Windows) front-matter block, not just LF', () => {
|
||||
// F9: the line-anchored regex needs LF after the opening `---`, so a Windows
|
||||
// file (`---\r\n…`) would slip past the strip and leak the front-matter into
|
||||
// the body. normalizeForeignMarkdown normalizes CRLF -> LF first.
|
||||
const out = normalizeForeignMarkdown(
|
||||
'---\r\ntitle: Foo\r\ntags: [a]\r\n---\r\n\r\n# Heading\r\n\r\nBody.',
|
||||
);
|
||||
expect(out).toBe('# Heading\n\nBody.');
|
||||
expect(out).not.toContain('title: Foo');
|
||||
expect(out).not.toContain('---');
|
||||
});
|
||||
|
||||
it('strips front-matter whose value contains a triple-dash (line-anchored)', () => {
|
||||
// F8: the block must close only on a `\n---` LINE, not the first inline
|
||||
// `---`. A value like `title: Q1 --- Q2` must not truncate the front-matter
|
||||
// and leak the rest (author/closing ---) into the body.
|
||||
const out = normalizeForeignMarkdown(
|
||||
'---\ntitle: Q1 --- Q2 results\nauthor: bob\n---\n\nReal body.',
|
||||
);
|
||||
expect(out).toBe('Real body.');
|
||||
expect(out).not.toContain('author: bob');
|
||||
expect(out).not.toContain('Q2 results');
|
||||
});
|
||||
});
|
||||
|
||||
describe('foreign markdown import acceptance (normalizer + canonical parser)', () => {
|
||||
const FOREIGN = [
|
||||
'# Doc',
|
||||
'',
|
||||
'Body refs [^c] and [^a] and [^b] and again [^a].',
|
||||
'',
|
||||
':::info',
|
||||
'A legacy callout.',
|
||||
':::',
|
||||
'',
|
||||
'| h1 | h2 |',
|
||||
'| --- | --- |',
|
||||
'| 1 | 2 |',
|
||||
'',
|
||||
'[^a]: note A',
|
||||
'[^b]: note B',
|
||||
'[^c]: note C',
|
||||
'[^z]: orphan note',
|
||||
].join('\n');
|
||||
|
||||
it('leaves no literal [^id] or ::: in the imported doc and re-exports canonically', async () => {
|
||||
const normalized = normalizeForeignMarkdown(FOREIGN);
|
||||
const doc = await markdownToProseMirror(normalized);
|
||||
const reexport = convertProseMirrorToMarkdown(doc);
|
||||
|
||||
// No foreign garbage leaks into the document.
|
||||
expect(reexport).not.toMatch(/\[\^/); // no reference footnote refs/defs
|
||||
expect(reexport).not.toContain(':::'); // no legacy callout fences
|
||||
|
||||
// Canonical forms are present.
|
||||
expect(reexport).toContain('^[note C]');
|
||||
expect(reexport).toContain('> [!info]');
|
||||
expect(reexport).toContain('| h1 | h2 |');
|
||||
|
||||
// Footnotes: ordered by first reference (C, A, B), reused [^a] deduped to one,
|
||||
// orphan [^z] dropped (it had no reference after normalization).
|
||||
const list = doc.content.find((n: any) => n.type === 'footnotesList');
|
||||
const bodies = list.content.map(
|
||||
(d: any) => d.content[0].content[0].text,
|
||||
);
|
||||
expect(bodies).toEqual(['note C', 'note A', 'note B']);
|
||||
expect(bodies).not.toContain('orphan note');
|
||||
expect(
|
||||
doc.content.filter((n: any) => n.type === 'footnotesList'),
|
||||
).toHaveLength(1);
|
||||
});
|
||||
});
|
||||
@@ -1,266 +0,0 @@
|
||||
/**
|
||||
* Foreign-markdown normalizer — an input-liberal / output-canonical adapter that
|
||||
* runs at the IMPORT boundary, BEFORE the canonical parser
|
||||
* (`markdownToProseMirror` from `@docmost/prosemirror-markdown`).
|
||||
*
|
||||
* The canonical parser is deliberately STRICT: it only understands Docmost's
|
||||
* canonical markdown surface (Obsidian-style `> [!type]` callouts, Pandoc/Obsidian
|
||||
* inline footnotes `^[body]`, lossless ` <!--img {...}-->` images, …).
|
||||
* Import, however, ingests FOREIGN files (GitHub/GFM, Notion, old Docmost
|
||||
* exports). Those use surfaces the canonical parser does not accept, most notably
|
||||
* GitHub-flavoured *reference* footnotes:
|
||||
*
|
||||
* Text with a note[^1] and another[^long].
|
||||
*
|
||||
* [^1]: The first definition.
|
||||
* [^long]: A second one.
|
||||
*
|
||||
* Left untouched, the parser does NOT recognise `[^id]` (it only parses `^[body]`),
|
||||
* so the reference leaks as literal text — and worse, the trailing `[^id]: def`
|
||||
* line is a valid CommonMark *link-reference definition*, so `[^id]` is silently
|
||||
* rendered as a bogus link. This normalizer rewrites reference footnotes into the
|
||||
* canonical inline form so the parser materialises real footnote nodes.
|
||||
*
|
||||
* This is a TEXT pre-pass, NOT a second parser fork: it does not re-implement any
|
||||
* converter logic. Callout surfaces (`:::type` and `> [!type]`) are intentionally
|
||||
* NOT touched here — the canonical parser already accepts BOTH natively (its
|
||||
* `preprocessCallouts` pass), so normalizing them would be redundant and would
|
||||
* only risk degrading the parser's nesting/code-fence-aware handling.
|
||||
*/
|
||||
|
||||
/** Matches a fenced code block delimiter (``` or ~~~), capturing the marker run. */
|
||||
const CODE_FENCE_RE = /^(\s*)(`{3,}|~{3,})/;
|
||||
|
||||
/**
|
||||
* Matches a GFM footnote DEFINITION line: `[^id]: body`. The id is any run of
|
||||
* non-`]` characters; the body is the remainder of the line (possibly empty).
|
||||
*/
|
||||
const FOOTNOTE_DEF_RE = /^\[\^([^\]]+)\]:[ \t]?(.*)$/;
|
||||
|
||||
/** True when a line is a code-fence delimiter that toggles fenced-code state. */
|
||||
function fenceMarker(line: string): string | null {
|
||||
const m = line.match(CODE_FENCE_RE);
|
||||
return m ? m[2] : null;
|
||||
}
|
||||
|
||||
/** True when a line is indented (leading space/tab) and not blank — a continuation. */
|
||||
function isIndentedContinuation(line: string): boolean {
|
||||
return /^[ \t]+\S/.test(line);
|
||||
}
|
||||
|
||||
function escapeRegExp(value: string): string {
|
||||
return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
|
||||
}
|
||||
|
||||
/**
|
||||
* Backslash-escape any square bracket in a footnote body before it is wrapped in
|
||||
* `^[...]`. The canonical inline-footnote tokenizer scans the body with bracket
|
||||
* balancing and closes on the first UNMATCHED `]`, so an unbalanced bracket in a
|
||||
* foreign definition (e.g. `[^1]: see item ] later`) would otherwise truncate the
|
||||
* footnote and leak the tail as literal text. Escaping every `[`/`]` makes the
|
||||
* body an inert run of characters — the tokenizer then closes only on our own
|
||||
* closing `]`. (A balanced `[link](url)` inside a body still round-trips because
|
||||
* the escaped form renders the literal brackets, which is the safe reading for a
|
||||
* footnote body; the alternative — brittle balance tracking — risks worse.)
|
||||
*/
|
||||
function escapeFootnoteBody(body: string): string {
|
||||
return body.replace(/[[\]]/g, '\\$&');
|
||||
}
|
||||
|
||||
/**
|
||||
* Rewrite every `[^id]` reference on a line to its `^[body]` form, but ONLY in the
|
||||
* text OUTSIDE inline-code spans. A `[^id]` inside backticks is literal code
|
||||
* content and must be preserved verbatim (a footnote ref never lives inside code).
|
||||
* We split the line on inline-code spans (paired backtick runs) and rewrite only
|
||||
* the non-code segments.
|
||||
*/
|
||||
// Above this length a single line is not split into inline-code spans (see
|
||||
// below). A genuine markdown line carrying a footnote reference is never tens of
|
||||
// KB; the cap only bypasses the inline-code protection for pathological lines.
|
||||
const INLINE_SPLIT_MAX_LINE = 8192;
|
||||
|
||||
function rewriteRefsOutsideInlineCode(
|
||||
line: string,
|
||||
replace: (text: string) => string,
|
||||
): string {
|
||||
// The inline-code split alternation `(`+)(?:(?!\1)[\s\S])*\1` backtracks
|
||||
// quadratically on a long UNCLOSED backtick run (its middle can consume the
|
||||
// rest of the line, then fail to find a closing run and retry from each
|
||||
// position). On an untrusted import this is a request-thread ReDoS. A real
|
||||
// footnote line is short, so for an oversized line we skip the inline-code
|
||||
// protection entirely and leave the line UNTOUCHED (rewriting it wholesale
|
||||
// could corrupt a `[^id]` that legitimately lives inside inline code). This is
|
||||
// a conservative bypass: an over-8KB line simply does not get its reference
|
||||
// footnotes inlined — acceptable for a pathological input.
|
||||
if (line.length > INLINE_SPLIT_MAX_LINE) return line;
|
||||
|
||||
// Alternation: an inline-code span (one or more backticks, then anything up to
|
||||
// the SAME run of backticks) OR a run of non-backtick text. Unterminated
|
||||
// backticks fall through as ordinary text (matched by the second branch on the
|
||||
// leftover), so a stray backtick never swallows the rest of the line.
|
||||
const parts = line.match(/(`+)(?:(?!\1)[\s\S])*\1|[^`]+|`+/g);
|
||||
if (!parts) return line;
|
||||
return parts
|
||||
.map((seg) => (seg.startsWith('`') ? seg : replace(seg)))
|
||||
.join('');
|
||||
}
|
||||
|
||||
/**
|
||||
* Convert GFM reference footnotes (`[^id]` + `[^id]: def`) into canonical inline
|
||||
* footnotes (`^[def]`).
|
||||
*
|
||||
* - Definitions are collected first (a leading `[^id]: text` line plus any
|
||||
* immediately-following indented continuation lines, joined with a space) and
|
||||
* removed from the output.
|
||||
* - Each in-text reference `[^id]` for which a definition was found is replaced by
|
||||
* `^[def]`. References with no matching definition are left literal (there is no
|
||||
* body to inline; the parser fails them open the same way).
|
||||
* - Code is respected on both passes: `[^id]` inside a fenced ``` / ~~~ block is
|
||||
* never rewritten and a `[^id]:` line inside a fence is never a definition; and
|
||||
* on the rewrite pass a `[^id]` inside an INLINE-code span (backticks) is left
|
||||
* literal too.
|
||||
* - The inlined body is bracket-escaped so an unbalanced `[`/`]` in a foreign
|
||||
* definition cannot truncate the resulting `^[...]` footnote.
|
||||
*
|
||||
* Deduplication / reference-ordering / orphan-dropping of the resulting footnotes
|
||||
* is handled downstream by the canonical parser (`assembleFootnotes`); this pass
|
||||
* only changes the surface syntax.
|
||||
*/
|
||||
function convertReferenceFootnotes(markdown: string): string {
|
||||
const lines = markdown.split('\n');
|
||||
|
||||
// Pass 1: collect definitions and mark their lines for removal.
|
||||
const defs = new Map<string, string>();
|
||||
const dropped = new Array<boolean>(lines.length).fill(false);
|
||||
let inFence = false;
|
||||
let fence = '';
|
||||
|
||||
for (let i = 0; i < lines.length; i++) {
|
||||
const line = lines[i];
|
||||
const marker = fenceMarker(line);
|
||||
if (inFence) {
|
||||
if (marker && marker[0] === fence[0] && marker.length >= fence.length) {
|
||||
inFence = false;
|
||||
fence = '';
|
||||
}
|
||||
continue;
|
||||
}
|
||||
if (marker) {
|
||||
inFence = true;
|
||||
fence = marker;
|
||||
continue;
|
||||
}
|
||||
|
||||
const def = line.match(FOOTNOTE_DEF_RE);
|
||||
if (!def) continue;
|
||||
|
||||
const id = def[1];
|
||||
const body: string[] = [def[2].trim()];
|
||||
dropped[i] = true;
|
||||
|
||||
// Consume immediately-following indented continuation lines (GFM lazy
|
||||
// continuation is not supported by design — keep it simple and predictable).
|
||||
let j = i + 1;
|
||||
while (j < lines.length && isIndentedContinuation(lines[j])) {
|
||||
body.push(lines[j].trim());
|
||||
dropped[j] = true;
|
||||
j++;
|
||||
}
|
||||
i = j - 1;
|
||||
|
||||
// Last definition wins for a duplicated id (matches CommonMark link-ref
|
||||
// semantics closely enough for a foreign-input adapter).
|
||||
defs.set(id, body.filter((s) => s.length > 0).join(' '));
|
||||
}
|
||||
|
||||
if (defs.size === 0) {
|
||||
return markdown;
|
||||
}
|
||||
|
||||
// ONE fixed, generic scanner regex — NOT one built from the definition ids.
|
||||
// It matches ANY `[^id]` shape, and the replacer decides per match via a map
|
||||
// lookup whether that id is a real definition (replace) or not (leave as-is).
|
||||
// This is genuinely O(total text) with no per-document regex compilation.
|
||||
//
|
||||
// Do NOT rebuild this as an alternation over `[...defs.keys()]`: a giant
|
||||
// `(id1|id2|...)` alternation over thousands of ids can blow the V8 regex
|
||||
// compiler's stack — a fatal, UNCATCHABLE "RegExpCompiler Allocation failed"
|
||||
// on prefix-chain ids (`a`, `aa`, `aaa`, ...) that kills the whole process
|
||||
// (worse than the earlier per-def thread-hang). A fixed scanner has no
|
||||
// id-dependent compilation cost and cannot blow up.
|
||||
const refRe = /\[\^([^\]]+)\]/g;
|
||||
const rewriteSegment = (segment: string): string =>
|
||||
segment.replace(refRe, (whole, id: string) => {
|
||||
const body = defs.get(id);
|
||||
// Only real definitions are inlined; an unknown id is left literal (same as
|
||||
// the old per-def loop, which simply never matched it).
|
||||
return body === undefined ? whole : `^[${escapeFootnoteBody(body)}]`;
|
||||
});
|
||||
|
||||
// Pass 2: rewrite in-text references, skipping fenced code and dropped lines.
|
||||
const out: string[] = [];
|
||||
inFence = false;
|
||||
fence = '';
|
||||
for (let i = 0; i < lines.length; i++) {
|
||||
if (dropped[i]) continue;
|
||||
let line = lines[i];
|
||||
|
||||
const marker = fenceMarker(line);
|
||||
if (inFence) {
|
||||
out.push(line);
|
||||
if (marker && marker[0] === fence[0] && marker.length >= fence.length) {
|
||||
inFence = false;
|
||||
fence = '';
|
||||
}
|
||||
continue;
|
||||
}
|
||||
if (marker) {
|
||||
inFence = true;
|
||||
fence = marker;
|
||||
out.push(line);
|
||||
continue;
|
||||
}
|
||||
|
||||
line = rewriteRefsOutsideInlineCode(line, rewriteSegment);
|
||||
out.push(line);
|
||||
}
|
||||
|
||||
return out.join('\n');
|
||||
}
|
||||
|
||||
/**
|
||||
* Strip a single leading YAML front-matter block (`---\n…\n---`). Foreign files
|
||||
* from Obsidian / Hugo / Jekyll / Notion — and Docmost's OWN git-sync page files
|
||||
* — open with front-matter that the canonical parser does not consume, so
|
||||
* without this it leaks into the body (and `title: Foo` above the closing `---`
|
||||
* renders as a setext `<h2>` that `extractTitleAndRemoveHeading` can hijack as
|
||||
* the page title). It is a no-op for front-matter-free input.
|
||||
*
|
||||
* LINE-ANCHORED (the same shape the canonical parser uses in
|
||||
* prosemirror-markdown/page-file.ts): the block opens only on `---\n` at the
|
||||
* very start and closes only on a `\n---` line. The retired editor-ext
|
||||
* `markdownToHtml` front-matter strip (removed in #347) closed on the FIRST
|
||||
* `---` ANYWHERE (an unanchored close), so a value
|
||||
* containing a triple-dash (e.g. `title: Q1 --- Q2`) truncated the front-matter
|
||||
* and leaked the rest into the body. An optional leading BOM is tolerated.
|
||||
*/
|
||||
const YAML_FRONT_MATTER_RE = /^\uFEFF?---\n[\s\S]*?\n---\n?/;
|
||||
|
||||
/**
|
||||
* Normalize a foreign markdown string into Docmost's canonical markdown surface
|
||||
* so the strict canonical parser accepts it losslessly: normalize line endings,
|
||||
* strip a leading YAML front-matter block, then rewrite GFM reference footnotes
|
||||
* into inline footnotes. Add further fixture-driven foreign-surface cases here as
|
||||
* they are found.
|
||||
*/
|
||||
export function normalizeForeignMarkdown(markdown: string): string {
|
||||
if (!markdown) return markdown;
|
||||
// Normalize CRLF -> LF FIRST. The line-anchored front-matter regex requires a
|
||||
// bare `\n` after the opening `---`, and convertReferenceFootnotes splits on
|
||||
// `\n`; a Windows/CRLF foreign file (`---\r\n…`) would otherwise slip past the
|
||||
// front-matter strip and leak into the body. The canonical parser
|
||||
// (page-file.ts parsePageFile) normalizes the same way before its FRONTMATTER_RE.
|
||||
const src = markdown.replace(/\r\n/g, '\n');
|
||||
const withoutFrontMatter = src.replace(YAML_FRONT_MATTER_RE, '').trimStart();
|
||||
return convertReferenceFootnotes(withoutFrontMatter);
|
||||
}
|
||||
+8
-2
@@ -139,13 +139,19 @@ describe('GeneralQueueProcessor — COMMENT_MARK_UPDATE (#399)', () => {
|
||||
);
|
||||
});
|
||||
|
||||
it('skips (no throw) when the comment row has vanished', async () => {
|
||||
it('reconcile (#496): comment row vanished → strips the orphan anchor mark', async () => {
|
||||
const { proc, collaborationGateway, commentRepo } = makeProc();
|
||||
commentRepo.findById.mockResolvedValue(undefined);
|
||||
|
||||
await expect(
|
||||
proc.process(job({ ...base, action: 'resolve', ts: 1000 })),
|
||||
).resolves.toBeUndefined();
|
||||
expect(collaborationGateway.handleYjsEvent).not.toHaveBeenCalled();
|
||||
// A resolve/unresolve mark job whose comment row is gone leaves a silent
|
||||
// orphan; the worker self-heals by stripping the anchor instead of returning.
|
||||
expect(collaborationGateway.handleYjsEvent).toHaveBeenCalledWith(
|
||||
'deleteCommentMark',
|
||||
'page.page-1',
|
||||
{ commentId: 'c-1', user: { id: 'user-1' } },
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -95,7 +95,8 @@ export class GeneralQueueProcessor
|
||||
* #399: apply a comment's inline-mark mirror in the collab Y.Doc, off the HTTP
|
||||
* critical path. Runs the SAME gateway path the synchronous comment.service
|
||||
* code used (byte-identical mark op):
|
||||
* - resolve / unresolve → resolveCommentMark (flip the `resolved` attribute);
|
||||
* - resolve / unresolve → resolveCommentMark (flip the `resolved` attribute),
|
||||
* OR strip an orphan anchor when the comment row has vanished (#496);
|
||||
* - delete → deleteCommentMark (strip the ephemeral-suggestion anchor #329).
|
||||
* The op is idempotent, so a BullMQ retry is safe. Throwing propagates to
|
||||
* WorkerHost → the job is retried and, on exhaustion, surfaces in failed-job
|
||||
@@ -133,7 +134,18 @@ export class GeneralQueueProcessor
|
||||
// of this resolve), skip it rather than flip the mark to a stale state.
|
||||
const comment = await this.commentRepo.findById(commentId);
|
||||
if (!comment) {
|
||||
// The comment vanished (e.g. hard-deleted) → nothing left to mirror.
|
||||
// #496 reconcile: the comment row is GONE (e.g. an ephemeral apply/dismiss
|
||||
// hard-deleted it while this resolve/unresolve mark job sat in the queue),
|
||||
// but its inline anchor may still live in the doc — a silent orphan mark
|
||||
// pointing at a comment that no longer exists. Self-heal by stripping it
|
||||
// instead of just returning: this closes the divergence the fire-and-forget
|
||||
// resolve/unresolve enqueue (comment.service resolveComment) could leave.
|
||||
// Idempotent — deleteCommentMark on an already-absent mark is a no-op.
|
||||
await this.getCollaborationGateway().handleYjsEvent(
|
||||
'deleteCommentMark',
|
||||
documentName,
|
||||
{ commentId, user },
|
||||
);
|
||||
return;
|
||||
}
|
||||
const wantResolved = action === 'resolve';
|
||||
|
||||
Reference in New Issue
Block a user