-
Notifications
You must be signed in to change notification settings - Fork 42
feat: add mysql file system #396
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a MySQL-backed filesystem stack: a FileSystemQualifier decorator, a MysqlFilesystem BackendProtocol implementation, an in-database AgentFS with chunked storage, plus supporting errors, guards, interfaces, config types, package deps, and extensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application (AppController)
participant Decorator as FileSystemQualifier
participant MFS as MysqlFilesystem
participant AFS as AgentFS
participant DB as MySQL
App->>Decorator: `@FileSystemQualifier`('foo') inject teggFilesystem
Decorator->>Decorator: QualifierUtil.addInjectQualifier(...)
Decorator->>MFS: resolve instance via getFileSystemConfig()
MFS->>MFS: init()
MFS->>AFS: AgentFS.fromDatabase(db)
AFS->>DB: create/ensure tables (fs_config, fs_inode, fs_dentry, fs_data, fs_symlink)
DB-->>AFS: ready
AFS-->>MFS: FS ready
MFS-->>App: ready
App->>MFS: write(path, content)
MFS->>AFS: writeFile(path, content)
AFS->>DB: insert/update inode, dentry, fs_data (chunked)
DB-->>AFS: success
AFS-->>MFS: success
MFS-->>App: WriteResult
App->>MFS: read(path)
MFS->>AFS: readFile(path)
AFS->>DB: select inode/dentry/fs_data chunks
DB-->>AFS: chunks
AFS->>AFS: reassemble chunks
AFS-->>MFS: content
MFS-->>App: formatted content
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @akitaSummer, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant new feature to the LangChain plugin: a MySQL-backed file system. This allows applications to manage and persist file-like data directly within a MySQL database, providing a robust and scalable solution for storing agent-related files. The implementation includes a core Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new MySQL-backed virtual filesystem, which is a significant feature. The implementation is comprehensive, including a detailed agentfs layer that mimics a standard filesystem API on top of a MySQL database, and extensive tests covering various filesystem operations.
My review has identified several critical issues, primarily related to database compatibility where SQLite-specific SQL syntax (PRAGMA, ON CONFLICT) is used with a MySQL datasource. There is also a critical security issue where a check to prevent operations on the root directory has been disabled. Additionally, I've found a high-severity performance issue due to a commented-out index creation and a logic bug in configuration handling.
I've also included some medium-severity suggestions to improve code maintainability by removing dead code, addressing the use of @ts-ignore, and simplifying repetitive test cleanup logic.
Addressing these points will greatly improve the robustness, security, and performance of this new feature.
| const upsertSql = SqlString.format(` | ||
| INSERT INTO fs_data (ino, chunk_index, data) VALUES (?, ?, ?) | ||
| ON CONFLICT(ino, chunk_index) DO UPDATE SET data = excluded.data | ||
| `, [ this.ino, chunkIdx, chunkData ]); | ||
| await this.db.query(upsertSql, [], { executeType: 'execute' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The upsertSql uses ON CONFLICT, which is SQLite-specific syntax. For MySQL, you should use ON DUPLICATE KEY UPDATE.
| const upsertSql = SqlString.format(` | |
| INSERT INTO fs_data (ino, chunk_index, data) VALUES (?, ?, ?) | |
| ON CONFLICT(ino, chunk_index) DO UPDATE SET data = excluded.data | |
| `, [ this.ino, chunkIdx, chunkData ]); | |
| await this.db.query(upsertSql, [], { executeType: 'execute' }); | |
| const upsertSql = SqlString.format(` | |
| INSERT INTO fs_data (ino, chunk_index, data) VALUES (?, ?, ?) | |
| ON DUPLICATE KEY UPDATE data = VALUES(data) | |
| `, [ this.ino, chunkIdx, chunkData ]); | |
| await this.db.query(upsertSql, [], { executeType: 'execute' }); |
| async fsync(): Promise<void> { | ||
| await this.db.query('PRAGMA synchronous = FULL', [], { executeType: 'execute' }); | ||
| await this.db.query('PRAGMA wal_checkpoint(TRUNCATE)', [], { executeType: 'execute' }); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fsync method uses PRAGMA statements, which are specific to SQLite. Since this filesystem is backed by MysqlDataSource, these queries will fail at runtime as MySQL does not support PRAGMA.
Forcing a sync to disk in MySQL is generally handled by transaction commits with appropriate durability settings (e.g., innodb_flush_log_at_trx_commit). Given the context of this method, it might be better to make this a no-op for MySQL or implement it using transaction semantics if required.
async fsync(): Promise<void> {
// fsync is not directly applicable to MySQL in the same way as file-based systems.
// InnoDB durability is controlled by server variables.
// This can be a no-op.
return Promise.resolve();
}| if (path === '/') { | ||
| // throw createFsError({ | ||
| // code: 'EPERM', | ||
| // syscall, | ||
| // path, | ||
| // message: 'operation not permitted on root directory', | ||
| // }); | ||
| console.log(syscall); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check to prevent operations on the root directory ('/') is commented out and replaced with a console.log. This is a significant security risk as it could allow destructive operations on the root of the virtual filesystem. The error-throwing logic should be restored.
if (path === '/') {
throw createFsError({
code: 'EPERM',
syscall,
path,
message: 'operation not permitted on root directory',
});
}| // await this.db.query(` | ||
| // CREATE INDEX idx_fs_dentry_parent ON fs_dentry(parent_ino, name); | ||
| // `, [], { executeType: 'execute' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CREATE INDEX statement for fs_dentry(parent_ino, name) is commented out. This index is crucial for performance when resolving paths, especially in deep directory structures. Without it, resolvePath and other path-based operations will perform full table scans on fs_dentry, leading to poor performance as the number of files grows. This index should be enabled. Using IF NOT EXISTS will prevent errors on subsequent initializations.
await this.db.query(`
CREATE INDEX IF NOT EXISTS idx_fs_dentry_parent ON fs_dentry(parent_ino, name);
`, [], { executeType: 'execute' });There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
补上。
| if (!config) { | ||
| throw new Error(`not found FsMiddleware config for ${filesystemName}`); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check if (!config) is incorrect. config is the ModuleConfig object which is unlikely to be falsy. The check should be on fsConfig to ensure that the configuration for the specific filesystem client exists. An incorrect check can lead to confusing runtime errors if the configuration is missing.
| if (!config) { | |
| throw new Error(`not found FsMiddleware config for ${filesystemName}`); | |
| } | |
| if (!fsConfig) { | |
| throw new Error(`not found FsMiddleware config for ${filesystemName}`); | |
| } |
| // const result = (await this.db.query(insertSql, [], { executeType: 'execute' }))[0]; | ||
| // const { ino } = result as { ino: number }; | ||
| // return Number(ino); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| // @ts-ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| // @ts-ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const delConfig = 'DELETE FROM fs_config'; | ||
| await mysql.query(delConfig, [], { executeType: 'execute' }); | ||
| const delData = 'DELETE FROM fs_data'; | ||
| await mysql.query(delData, [], { executeType: 'execute' }); | ||
| const delDentry = 'DELETE FROM fs_dentry'; | ||
| await mysql.query(delDentry, [], { executeType: 'execute' }); | ||
| const delInode = 'DELETE FROM fs_inode'; | ||
| await mysql.query(delInode, [], { executeType: 'execute' }); | ||
| const delSymlink = 'DELETE FROM fs_symlink'; | ||
| await mysql.query(delSymlink, [], { executeType: 'execute' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The database cleanup logic in this after hook is repetitive. You can simplify this by iterating over a list of table names. This would make the code more concise and easier to maintain if more tables are added in the future.
| const delConfig = 'DELETE FROM fs_config'; | |
| await mysql.query(delConfig, [], { executeType: 'execute' }); | |
| const delData = 'DELETE FROM fs_data'; | |
| await mysql.query(delData, [], { executeType: 'execute' }); | |
| const delDentry = 'DELETE FROM fs_dentry'; | |
| await mysql.query(delDentry, [], { executeType: 'execute' }); | |
| const delInode = 'DELETE FROM fs_inode'; | |
| await mysql.query(delInode, [], { executeType: 'execute' }); | |
| const delSymlink = 'DELETE FROM fs_symlink'; | |
| await mysql.query(delSymlink, [], { executeType: 'execute' }); | |
| const tables = [ | |
| 'fs_config', | |
| 'fs_data', | |
| 'fs_dentry', | |
| 'fs_inode', | |
| 'fs_symlink', | |
| ]; | |
| for (const table of tables) { | |
| await mysql.query(`DELETE FROM ${table}`, [], { executeType: 'execute' }); | |
| } |
| // const fd = await this.agentFs.open( | ||
| // resolvedPath, | ||
| // ); | ||
| // try { | ||
| // content = await fd.readFile({ encoding: 'utf-8' }); | ||
| // } finally { | ||
| // await fd.close(); | ||
| // } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugin/langchain/test/fixtures/sse-mcp-server/http.ts (2)
41-43: Variable shadowing bug: exportedhttpServeris never assigned.The outer
let httpServer(line 41) is exported but the innerconst httpServer(line 43) shadows it. The exported variable will always beundefined, making it unusable by consumers.Proposed fix
-export let httpServer; +export let httpServer: http.Server; export async function startSSEServer(port = 17233) { - const httpServer = http.createServer(async (req, res) => { + httpServer = http.createServer(async (req, res) => {
73-74:stopSSEServerdoes not close the HTTP server.Only the MCP server is closed. The HTTP server created in
startSSEServercontinues listening, which can cause port conflicts or resource leaks in tests.Proposed fix
export async function stopSSEServer() { server.close(); + if (httpServer) { + httpServer.close(); + } }
🤖 Fix all issues with AI agents
In `@core/langchain-decorator/src/qualifier/FileSystemQualifier.ts`:
- Line 1: The import in FileSystemQualifier.ts incorrectly pulls QualifierUtil
from '@eggjs/tegg'; change the module specifier to '@eggjs/core-decorator' so
QualifierUtil is imported from the correct package. Locate the import statement
that mentions QualifierUtil in the FileSystemQualifier.ts module and update the
string literal to '@eggjs/core-decorator', keeping the imported symbol name
unchanged.
In `@plugin/langchain/lib/filesystem/agentfs/agentfs.ts`:
- Around line 217-233: The fstat implementation treats this.db.query result as a
single row object but db.query returns an array; modify the
SqlString.format/query call in the fstat method to await this.db.query(sql) as
an array type, then extract the first element (e.g., const row = rows[0]) and
handle the case where rows.length === 0 (same pattern used in
getSize/write/truncate). Update the local type to reflect an array of rows with
fields ino, mode, nlink, uid, gid, size, atime, mtime, ctime and use the first
element as the row variable that the subsequent !row check uses.
- Around line 211-214: The fsync method currently runs SQLite-specific PRAGMA
statements via this.db.query (PRAGMA synchronous and PRAGMA wal_checkpoint)
which will fail for MySQL; change fsync in agentfs.ts to detect the underlying
DB/dialect (e.g., inspect the data source/driver or a dialect property on
this.db or its client) and only execute those PRAGMA queries when running
against SQLite, otherwise perform a safe no-op (return immediately) or implement
MySQL-specific durability steps if required; update references to this.db.query
inside fsync to be conditional so MySQL paths never execute PRAGMA statements.
- Around line 675-679: The timestamps from this.stat (fs.ctime and fs.mtime) are
in seconds but are passed directly to new Date() in the files construction,
producing incorrect dates; update the code that builds files[...] (the block
using readFileLines, stat, stats.isFile and the local fs variable) to multiply
fs.ctime and fs.mtime by 1000 before calling new Date(...) (e.g., use new
Date(fs.ctime * 1000) and new Date(fs.mtime * 1000)) so created_at and
modified_at become correct ISO strings.
- Around line 149-153: The INSERT uses Postgres/SQLite "ON CONFLICT ... DO
UPDATE" which is invalid for MySQL—change the upsert in the agentfs.ts code that
builds upsertSql to use MySQL's "ON DUPLICATE KEY UPDATE" on the fs_data table
(primary key is (ino, chunk_index)); keep using SqlString.format for the
parameter substitution for this. Concretely, update the SQL string constructed
in the upsertSql variable so it inserts (ino, chunk_index, data) and appends "ON
DUPLICATE KEY UPDATE data = VALUES(data)" (or the MySQL equivalent) before
calling this.db.query, ensuring the rest of the call (this.ino, chunkIdx,
chunkData) stays as the formatted parameters.
In `@plugin/langchain/lib/filesystem/agentfs/guards.ts`:
- Around line 44-53: The guard function assertNotRoot currently logs the syscall
on root-path checks instead of throwing an FS error; replace the
console.log(syscall) with a thrown error using createFsError so callers receive
an EPERM. Specifically, inside assertNotRoot when path === '/' throw
createFsError({ code: 'EPERM', syscall: (the FsSyscall argument), path, message:
'operation not permitted on root directory' }) (keeping the FsSyscall parameter
name and structure), ensuring the thrown error matches the other filesystem
errors the codebase expects.
In `@plugin/langchain/lib/filesystem/MysqlFilesystem.ts`:
- Around line 360-365: The code currently calls new RegExp(pattern) directly
(seen in MysqlFilesystem.ts where variables pattern and regex are used), which
is vulnerable to ReDoS from untrusted LLM input; before compiling, validate and
sanitize the pattern by either using a safe-regex library (e.g., safe-regex2) to
test the pattern for catastrophic backtracking, or enforce strict limits (max
length and reject nested quantifiers/complex constructs) and only then call new
RegExp; if the pattern fails the safety check, return a clear error string
similar to the existing Invalid regex response instead of compiling.
- Around line 155-167: The timestamps being passed into new Date() in
MysqlFilesystem.ts are in seconds (AgentFS uses seconds), so update the
timestamp conversions in the lsInfo() and readRaw() methods to multiply the
stored time values by 1000 before creating Date objects; e.g., change new
Date(entryStat.mtime) to new Date(entryStat.mtime * 1000) (apply the same change
for all occurrences in lsInfo() and readRaw(), and for any other places in
MysqlFilesystem.ts where AgentFS-style epoch seconds are converted to Date).
In `@plugin/langchain/lib/util.ts`:
- Around line 27-34: getFileSystemConfig currently checks `if (!config)` which
is incorrect and allows `fsConfig` (from
`config.filesystem?.clients[filesystemName]`) to be undefined; update the guard
to validate `fsConfig` instead of `config` and throw a clear error referencing
`filesystemName` if the client is missing, ensuring the function does not return
`undefined` for `fsConfig`. Use the existing `filesystemName` lookup and
`config.filesystem?.clients` to locate the client and replace the faulty `if
(!config)` block with a check like `if (!fsConfig) throw new Error(...)`,
removing the unsafe non-null assertion (`fsConfig!`) thereafter.
In `@plugin/langchain/typings/index.d.ts`:
- Around line 113-128: The schema FileSystemConfigModuleConfigSchema currently
sets the schema option name to 'FsMiddleware', causing mismatch with the config
key 'filesystem'; update the Type.Object options for
FileSystemConfigModuleConfigSchema to use name: 'filesystem' (so error messages
and docs reference the config key consistently, mirroring the ChatModel pattern)
and leave the rest of the schema unchanged.
🧹 Nitpick comments (7)
plugin/langchain/test/fixtures/sse-mcp-server/http.ts (3)
16-18: Consider fixing the type mismatch instead of suppressing it.Using
@ts-ignoremasks the underlying type incompatibility. TheinputSchemaformat may not match whatregisterToolexpects from the MCP SDK. For test fixtures this is acceptable, but verify the schema format is actually correct at runtime.
56-65: Missing null check forsessionIdand transport lookup.If
sessionIdis missing from the query string or if no transport exists for that session, this will throw at runtime. Consider adding defensive checks.Proposed fix
} else if (req.method === 'POST') { const sessionId = url.searchParams.get('sessionId'); - // const chunks: Buffer[] = []; - // for await (const chunk of req) { - // chunks.push(chunk); - // } - // const body = JSON.parse(Buffer.concat(chunks).toString('utf-8')); - const transport = transports[sessionId!] as SSEServerTransport; + if (!sessionId || !transports[sessionId]) { + res.statusCode = 400; + res.end('Invalid or missing sessionId'); + return; + } + const transport = transports[sessionId] as SSEServerTransport; await transport.handlePostMessage(req, res);
38-39: Add proper type annotations fortransportsandheaders.Using
{}as the type loses type safety. Consider adding explicit record types.Proposed fix
-const transports = {}; -export const headers = {}; +const transports: Record<string, SSEServerTransport> = {}; +export const headers: Record<string, Record<string, unknown[]>> = {};plugin/langchain/test/fixtures/apps/langchain/app/modules/bar/service/Graph.ts (2)
45-47: Prefer@ts-expect-errorover@ts-ignorewith an explanation.Using
@ts-ignoresilently suppresses any TypeScript error, including future unrelated ones.@ts-expect-erroris safer because it fails when the expected error no longer exists (e.g., after fixing the underlying type issue).Consider investigating why the type mismatch occurs—it may indicate a missing or incorrect type definition in
ToolArgsSchemathat should be fixed upstream rather than suppressed here.Suggested change
- // eslint-disable-next-line `@typescript-eslint/ban-ts-comment` - // `@ts-ignore` + // `@ts-expect-error` - ToolArgsSchema type definition doesn't match zod schema inference
67-67: PreferinstanceofoverisPrototypeOffor type checking.
ToolMessage.prototype.isPrototypeOf(lastMessage)is less readable and slightly slower thanlastMessage instanceof ToolMessage. Theinstanceofoperator is the idiomatic way to check class instances in JavaScript/TypeScript.Suggested change
- if (ToolMessage.prototype.isPrototypeOf(lastMessage)) { + if (lastMessage instanceof ToolMessage) {core/langchain-decorator/src/qualifier/FileSystemQualifier.ts (1)
6-9: Consider adding explicit types for improved type safety.The decorator parameters use implicit
anytypes. Adding explicit types improves IDE support and catches potential misuse at compile time. This aligns with the pattern used in other qualifiers in this codebase.Suggested improvement
export function FileSystemQualifier(fsMiddlewareName: string) { - return function(target: any, propertyKey?: PropertyKey, parameterIndex?: number) { + return function(target: object, propertyKey?: PropertyKey, parameterIndex?: number): void { QualifierUtil.addInjectQualifier(target, propertyKey, parameterIndex, FileSystemQualifierAttribute, fsMiddlewareName); }; }plugin/langchain/test/llm.test.ts (1)
1-4: UseMockModuleContextScope/mockModuleContextScopefor plugin tests.The plugin test harness is expected to use context scopes from
egg-mockrather than onlymm.app(...), to ensure proper plugin context behavior during tests. Based on learnings, please migrate toMockModuleContextScopeormockModuleContextScope.
| @@ -0,0 +1,10 @@ | |||
| import { QualifierUtil } from '@eggjs/tegg'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
Import from @eggjs/core-decorator instead of @eggjs/tegg.
As per coding guidelines, in core/* packages, decorator utilities like QualifierUtil should be imported from @eggjs/core-decorator, not @eggjs/tegg.
Suggested fix
-import { QualifierUtil } from '@eggjs/tegg';
+import { QualifierUtil } from '@eggjs/core-decorator';🤖 Prompt for AI Agents
In `@core/langchain-decorator/src/qualifier/FileSystemQualifier.ts` at line 1, The
import in FileSystemQualifier.ts incorrectly pulls QualifierUtil from
'@eggjs/tegg'; change the module specifier to '@eggjs/core-decorator' so
QualifierUtil is imported from the correct package. Locate the import statement
that mentions QualifierUtil in the FileSystemQualifier.ts module and update the
string literal to '@eggjs/core-decorator', keeping the imported symbol name
unchanged.
| const upsertSql = SqlString.format(` | ||
| INSERT INTO fs_data (ino, chunk_index, data) VALUES (?, ?, ?) | ||
| ON CONFLICT(ino, chunk_index) DO UPDATE SET data = excluded.data | ||
| `, [ this.ino, chunkIdx, chunkData ]); | ||
| await this.db.query(upsertSql, [], { executeType: 'execute' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
# Find the target file and understand its context
find . -type f -name "agentfs.ts" | head -5Repository: eggjs/tegg
Length of output: 106
🏁 Script executed:
# Look for database configuration files
find . -type f -name "*.ts" -o -name "*.json" | xargs grep -l "mysql\|sqlite\|postgres" | head -20Repository: eggjs/tegg
Length of output: 892
🏁 Script executed:
# Check the schema/migration files to understand the database being used
find . -type f \( -name "*.sql" -o -name "*migration*" -o -name "*schema*" \) | head -20Repository: eggjs/tegg
Length of output: 1820
🏁 Script executed:
# Search for database connection setup
rg -i "mysql|sqlite|postgres|pool|database" --type ts -A 3 | head -100Repository: eggjs/tegg
Length of output: 6976
🏁 Script executed:
# Read the agentfs.ts file around lines 149-153
sed -n '140,160p' ./plugin/langchain/lib/filesystem/agentfs/agentfs.tsRepository: eggjs/tegg
Length of output: 763
🏁 Script executed:
# Search for all ON CONFLICT usages in the codebase
rg "ON CONFLICT" --type ts -B 2 -A 2Repository: eggjs/tegg
Length of output: 609
🏁 Script executed:
# Check the MysqlFilesystem.ts for context
head -100 ./plugin/langchain/lib/filesystem/MysqlFilesystem.tsRepository: eggjs/tegg
Length of output: 3395
🏁 Script executed:
# View more context around the agentfs.ts file to check for other MySQL incompatibilities
cat -n ./plugin/langchain/lib/filesystem/agentfs/agentfs.ts | head -200Repository: eggjs/tegg
Length of output: 8087
🏁 Script executed:
# Search for PRAGMA or other SQLite-specific commands in agentfs.ts
rg "PRAGMA|excluded\.|rowid" ./plugin/langchain/lib/filesystem/agentfs/agentfs.ts -B 2 -A 2Repository: eggjs/tegg
Length of output: 574
🏁 Script executed:
# Check if there are any other ON CONFLICT patterns or SQLite-specific code
rg "fsync|fstat|readallFiles" ./plugin/langchain/lib/filesystem/agentfs/agentfs.ts -B 3 -A 8Repository: eggjs/tegg
Length of output: 1432
🏁 Script executed:
# Get the complete fstat() implementation
sed -n '206,230p' ./plugin/langchain/lib/filesystem/agentfs/agentfs.tsRepository: eggjs/tegg
Length of output: 780
🏁 Script executed:
# Search for timestamp handling in the file
rg "atime|mtime|ctime|Date.now|new Date" ./plugin/langchain/lib/filesystem/agentfs/agentfs.ts -B 1 -A 1Repository: eggjs/tegg
Length of output: 3423
🏁 Script executed:
# Verify the actual return type of db.query()
rg "query.*:" ./plugin/langchain/lib/filesystem/agentfs/agentfs.ts | head -20Repository: eggjs/tegg
Length of output: 1587
🌐 Web query:
MySQL ON DUPLICATE KEY UPDATE syntax correct usage data column
💡 Result:
In INSERT ... ON DUPLICATE KEY UPDATE, the “new” values (the ones you attempted to insert) should be referenced using a row alias (recommended), not VALUES(col) (deprecated in MySQL 8.0.20+). [1], [2]
Recommended (MySQL 8.0.20+ / 8.4+): use a row alias
INSERT INTO t (id, data)
VALUES (1, 'abc') AS new
ON DUPLICATE KEY UPDATE
data = new.data;MySQL explicitly supports using a row alias (optionally with column aliases) to refer to the row being inserted in the ON DUPLICATE KEY UPDATE clause. [3]
With column aliases (useful if you insert expressions)
INSERT INTO t (id, data)
VALUES (1, 'abc') AS new(id, data)
ON DUPLICATE KEY UPDATE
data = new.data;[3]
Legacy form (works but deprecated): VALUES(data)
INSERT INTO t (id, data)
VALUES (1, 'abc')
ON DUPLICATE KEY UPDATE
data = VALUES(data);This VALUES() usage is deprecated and “subject to removal in a future version,” so prefer the alias form. [1], [2]
Notes
- The
UPDATEhappens when the insert would violate a PRIMARY KEY or UNIQUE index. [3]
Sources:
[1] MySQL 8.0 “What Is New” (deprecates VALUES() in ODKU) (docs.oracle.com)
[2] MySQL 5.7 manual: INSERT ... ON DUPLICATE KEY UPDATE details (legacy behavior) (docs.oracle.com)
[3] MySQL 8.4 Reference Manual: INSERT syntax + row alias support (dev.mysql.com)
Citations:
- 1: https://docs.oracle.com/cd/E17952_01/mysql-8.0-en/mysql-nutshell.html?utm_source=openai
- 2: https://docs.oracle.com/cd/E17952_01/mysql-5.7-en/insert-on-duplicate.html?utm_source=openai
- 3: https://dev.mysql.com/doc/refman/en/insert.html?utm_source=openai
🏁 Script executed:
# Get the complete fstat() method to see how the row is used
sed -n '218,245p' ./plugin/langchain/lib/filesystem/agentfs/agentfs.tsRepository: eggjs/tegg
Length of output: 701
🏁 Script executed:
# Check if there are any other database-specific issues
rg "PRAGMA|excluded\." ./plugin/langchain/lib/filesystem/agentfs/ -nRepository: eggjs/tegg
Length of output: 475
MySQL upsert syntax is invalid here.
ON CONFLICT ... DO UPDATE is SQLite/Postgres syntax and will fail on MySQL. Use ON DUPLICATE KEY UPDATE for the (ino, chunk_index) primary key.
✅ Proposed fix (MySQL upsert)
const upsertSql = SqlString.format(`
INSERT INTO fs_data (ino, chunk_index, data) VALUES (?, ?, ?)
- ON CONFLICT(ino, chunk_index) DO UPDATE SET data = excluded.data
+ ON DUPLICATE KEY UPDATE data = VALUES(data)
`, [ this.ino, chunkIdx, chunkData ]);🤖 Prompt for AI Agents
In `@plugin/langchain/lib/filesystem/agentfs/agentfs.ts` around lines 149 - 153,
The INSERT uses Postgres/SQLite "ON CONFLICT ... DO UPDATE" which is invalid for
MySQL—change the upsert in the agentfs.ts code that builds upsertSql to use
MySQL's "ON DUPLICATE KEY UPDATE" on the fs_data table (primary key is (ino,
chunk_index)); keep using SqlString.format for the parameter substitution for
this. Concretely, update the SQL string constructed in the upsertSql variable so
it inserts (ino, chunk_index, data) and appends "ON DUPLICATE KEY UPDATE data =
VALUES(data)" (or the MySQL equivalent) before calling this.db.query, ensuring
the rest of the call (this.ino, chunkIdx, chunkData) stays as the formatted
parameters.
| async fsync(): Promise<void> { | ||
| await this.db.query('PRAGMA synchronous = FULL', [], { executeType: 'execute' }); | ||
| await this.db.query('PRAGMA wal_checkpoint(TRUNCATE)', [], { executeType: 'execute' }); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the file mentioned in the review
find . -path "*plugin/langchain/lib/filesystem/agentfs/agentfs.ts" -type fRepository: eggjs/tegg
Length of output: 106
🏁 Script executed:
# Also search for the file by name in case the path is slightly different
fd -e ts "agentfs.ts"Repository: eggjs/tegg
Length of output: 104
🏁 Script executed:
# Read the agentfs.ts file to see the context around lines 211-214
wc -l plugin/langchain/lib/filesystem/agentfs/agentfs.tsRepository: eggjs/tegg
Length of output: 109
🏁 Script executed:
# Get the content around lines 211-214
sed -n '200,225p' plugin/langchain/lib/filesystem/agentfs/agentfs.tsRepository: eggjs/tegg
Length of output: 879
🏁 Script executed:
# Let's also check the imports and class definition to understand the db instance
head -50 plugin/langchain/lib/filesystem/agentfs/agentfs.tsRepository: eggjs/tegg
Length of output: 1235
🏁 Script executed:
# Check if there are any database type checks or conditionals related to MySQL vs SQLite
grep -n "PRAGMA\|pragma" plugin/langchain/lib/filesystem/agentfs/agentfs.tsRepository: eggjs/tegg
Length of output: 239
🏁 Script executed:
# Look for any database type detection logic in the file
grep -n "MysqlDataSource\|sqlite\|mysql" plugin/langchain/lib/filesystem/agentfs/agentfs.tsRepository: eggjs/tegg
Length of output: 448
🏁 Script executed:
# Check the entire fsync implementation and surrounding context
sed -n '205,220p' plugin/langchain/lib/filesystem/agentfs/agentfs.tsRepository: eggjs/tegg
Length of output: 589
🏁 Script executed:
# Search for any comments or documentation about database support
grep -B5 -A5 "fsync" plugin/langchain/lib/filesystem/agentfs/agentfs.ts | head -30Repository: eggjs/tegg
Length of output: 395
SQLite PRAGMA statements will fail on MySQL.
The code uses MysqlDataSource from @eggjs/dal-runtime but executes SQLite-specific commands: PRAGMA synchronous and PRAGMA wal_checkpoint(TRUNCATE). These commands are not supported by MySQL and will throw errors at runtime. Either implement a no-op or add MySQL-specific durability logic if needed.
Proposed fix (safe no-op)
async fsync(): Promise<void> {
- await this.db.query('PRAGMA synchronous = FULL', [], { executeType: 'execute' });
- await this.db.query('PRAGMA wal_checkpoint(TRUNCATE)', [], { executeType: 'execute' });
+ // MySQL doesn't support PRAGMA; no-op or implement a MySQL-specific durability step if needed.
+ return;
}🤖 Prompt for AI Agents
In `@plugin/langchain/lib/filesystem/agentfs/agentfs.ts` around lines 211 - 214,
The fsync method currently runs SQLite-specific PRAGMA statements via
this.db.query (PRAGMA synchronous and PRAGMA wal_checkpoint) which will fail for
MySQL; change fsync in agentfs.ts to detect the underlying DB/dialect (e.g.,
inspect the data source/driver or a dialect property on this.db or its client)
and only execute those PRAGMA queries when running against SQLite, otherwise
perform a safe no-op (return immediately) or implement MySQL-specific durability
steps if required; update references to this.db.query inside fsync to be
conditional so MySQL paths never execute PRAGMA statements.
| const sql = SqlString.format(` | ||
| SELECT ino, mode, nlink, uid, gid, size, atime, mtime, ctime | ||
| FROM fs_inode WHERE ino = ? | ||
| `, [ this.ino ]); | ||
| const row = await this.db.query(sql) as { | ||
| ino: number; | ||
| mode: number; | ||
| nlink: number; | ||
| uid: number; | ||
| gid: number; | ||
| size: number; | ||
| atime: number; | ||
| mtime: number; | ||
| ctime: number; | ||
| } | undefined; | ||
|
|
||
| if (!row) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file and understand the context around the flagged code
head -n 250 plugin/langchain/lib/filesystem/agentfs/agentfs.ts | tail -n 50Repository: eggjs/tegg
Length of output: 1453
🏁 Script executed:
# Find the db definition and how it's initialized in this file
rg -A 5 -B 5 'this\.db' plugin/langchain/lib/filesystem/agentfs/agentfs.ts | head -100Repository: eggjs/tegg
Length of output: 3521
🏁 Script executed:
# Search for db.query usage patterns in the codebase to understand return type
rg 'db\.query' --type ts -A 3 -B 1 | head -80Repository: eggjs/tegg
Length of output: 5677
fstat queries the database incorrectly.
db.query returns an array of rows, but this code treats it as a single object. This inconsistency with other queries in the same file (e.g., getSize, write, truncate) will cause incorrect stats or type errors. Extract the first row using array indexing like other queries in this class.
✅ Proposed fix
- const row = await this.db.query(sql) as {
+ const row = (await this.db.query(sql) as {
ino: number;
mode: number;
nlink: number;
uid: number;
gid: number;
size: number;
atime: number;
mtime: number;
ctime: number;
- } | undefined;
+ }[])[0];🤖 Prompt for AI Agents
In `@plugin/langchain/lib/filesystem/agentfs/agentfs.ts` around lines 217 - 233,
The fstat implementation treats this.db.query result as a single row object but
db.query returns an array; modify the SqlString.format/query call in the fstat
method to await this.db.query(sql) as an array type, then extract the first
element (e.g., const row = rows[0]) and handle the case where rows.length === 0
(same pattern used in getSize/write/truncate). Update the local type to reflect
an array of rows with fields ino, mode, nlink, uid, gid, size, atime, mtime,
ctime and use the first element as the row variable that the subsequent !row
check uses.
| const { name, stats } = entry; | ||
| if (stats.isFile()) { | ||
| const content = await this.readFileLines(path + '/' + name, 'utf8'); | ||
| const fs = await this.stat(path + '/' + name); | ||
| files[path + '/' + name] = { content, created_at: new Date(fs.ctime).toISOString(), modified_at: new Date(fs.mtime).toISOString() }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file structure and find the stat method
fd "agentfs.ts" --type fRepository: eggjs/tegg
Length of output: 104
🏁 Script executed:
# Read the file to understand the stat() method and the context around lines 675-679
cat -n plugin/langchain/lib/filesystem/agentfs/agentfs.ts | sed -n '660,690p'Repository: eggjs/tegg
Length of output: 1388
🏁 Script executed:
# Find the stat method definition to see what it returns
ast-grep --pattern $'stat($$$) {
$$$
}'Repository: eggjs/tegg
Length of output: 36
🏁 Script executed:
# Search for the stat method definition in the file
rg "async stat\(" plugin/langchain/lib/filesystem/agentfs/agentfs.ts -A 10Repository: eggjs/tegg
Length of output: 425
🏁 Script executed:
# Also search for stat method more broadly
rg "stat\s*\(" plugin/langchain/lib/filesystem/agentfs/agentfs.ts -B 2 -A 5Repository: eggjs/tegg
Length of output: 1277
🏁 Script executed:
# Check the full file to understand the class structure and find stat method
wc -l plugin/langchain/lib/filesystem/agentfs/agentfs.tsRepository: eggjs/tegg
Length of output: 109
🏁 Script executed:
# Read the complete stat() method implementation
cat -n plugin/langchain/lib/filesystem/agentfs/agentfs.ts | sed -n '540,600p'Repository: eggjs/tegg
Length of output: 2510
🏁 Script executed:
# Search for Stats type definition
rg "type Stats\|interface Stats" plugin/langchain/lib/filesystem/agentfs/agentfs.ts -A 10Repository: eggjs/tegg
Length of output: 36
🏁 Script executed:
# Also check for imports and Stats usage
rg "Stats" plugin/langchain/lib/filesystem/agentfs/agentfs.ts | head -20Repository: eggjs/tegg
Length of output: 361
🏁 Script executed:
# Find createStats function definition
rg "function createStats|const createStats|export.*createStats" plugin/langchain/lib/filesystem/agentfs/agentfs.ts -B 2 -A 15Repository: eggjs/tegg
Length of output: 36
🏁 Script executed:
# Search for Stats type definition and imports
rg "^import|^export" plugin/langchain/lib/filesystem/agentfs/agentfs.ts | grep -i statsRepository: eggjs/tegg
Length of output: 36
🏁 Script executed:
# Look at the beginning of the file for imports and type definitions
cat -n plugin/langchain/lib/filesystem/agentfs/agentfs.ts | head -80Repository: eggjs/tegg
Length of output: 2871
🏁 Script executed:
# Find the interface file
fd "interface.ts" plugin/langchain/lib/filesystem/Repository: eggjs/tegg
Length of output: 106
🏁 Script executed:
# Look for createStats function in the interface file
rg "createStats" plugin/langchain/lib/filesystem/ -A 20Repository: eggjs/tegg
Length of output: 8160
🏁 Script executed:
# Read the interface file to see the Stats and StatsData types
cat -n plugin/langchain/lib/filesystem/agentfs/interface.ts | head -120Repository: eggjs/tegg
Length of output: 3681
🏁 Script executed:
# Check how timestamps are being stored and used in other parts of the code
rg "Date\.now\|Math\.floor" plugin/langchain/lib/filesystem/agentfs/ -B 2 -A 2Repository: eggjs/tegg
Length of output: 36
🏁 Script executed:
# Check the database schema initialization to understand the timestamp format
rg "CREATE TABLE|atime|mtime|ctime" plugin/langchain/lib/filesystem/ -B 2 -A 2Repository: eggjs/tegg
Length of output: 15123
Multiply timestamps by 1000 before passing to Date constructor.
Line 679 stores fs.ctime and fs.mtime as seconds (see Math.floor(Date.now() / 1000) pattern used consistently throughout for database storage), but the Date constructor expects milliseconds.
🕒 Proposed fix
- files[path + '/' + name] = { content, created_at: new Date(fs.ctime).toISOString(), modified_at: new Date(fs.mtime).toISOString() };
+ files[path + '/' + name] = { content, created_at: new Date(fs.ctime * 1000).toISOString(), modified_at: new Date(fs.mtime * 1000).toISOString() };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { name, stats } = entry; | |
| if (stats.isFile()) { | |
| const content = await this.readFileLines(path + '/' + name, 'utf8'); | |
| const fs = await this.stat(path + '/' + name); | |
| files[path + '/' + name] = { content, created_at: new Date(fs.ctime).toISOString(), modified_at: new Date(fs.mtime).toISOString() }; | |
| const { name, stats } = entry; | |
| if (stats.isFile()) { | |
| const content = await this.readFileLines(path + '/' + name, 'utf8'); | |
| const fs = await this.stat(path + '/' + name); | |
| files[path + '/' + name] = { content, created_at: new Date(fs.ctime * 1000).toISOString(), modified_at: new Date(fs.mtime * 1000).toISOString() }; |
🤖 Prompt for AI Agents
In `@plugin/langchain/lib/filesystem/agentfs/agentfs.ts` around lines 675 - 679,
The timestamps from this.stat (fs.ctime and fs.mtime) are in seconds but are
passed directly to new Date() in the files construction, producing incorrect
dates; update the code that builds files[...] (the block using readFileLines,
stat, stats.isFile and the local fs variable) to multiply fs.ctime and fs.mtime
by 1000 before calling new Date(...) (e.g., use new Date(fs.ctime * 1000) and
new Date(fs.mtime * 1000)) so created_at and modified_at become correct ISO
strings.
| results.push({ | ||
| path: fullPath, | ||
| is_dir: false, | ||
| size: entryStat.size, | ||
| modified_at: new Date(entryStat.mtime).toString(), | ||
| }); | ||
| } else if (isDir) { | ||
| results.push({ | ||
| path: fullPath + path.sep, | ||
| is_dir: true, | ||
| size: 0, | ||
| modified_at: new Date(entryStat.mtime).toString(), | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the relevant file to understand the context
cat -n plugin/langchain/lib/filesystem/MysqlFilesystem.ts | head -200 | tail -80Repository: eggjs/tegg
Length of output: 3072
🏁 Script executed:
# Search for readRaw method and other timestamp usages
rg -n "readRaw|entryStat|mtime|ctime" plugin/langchain/lib/filesystem/MysqlFilesystem.tsRepository: eggjs/tegg
Length of output: 760
🏁 Script executed:
# Check what AgentFS is and how it stores timestamps
find . -name "*AgentFS*" -o -name "*agentfs*" | head -20Repository: eggjs/tegg
Length of output: 148
🏁 Script executed:
# Look for AgentFS definition or usage
rg -n "AgentFS\|new Date.*mtime\|new Date.*ctime" --type ts --type tsx | head -30Repository: eggjs/tegg
Length of output: 81
🏁 Script executed:
# Read the agentfs.ts file to understand timestamp format
cat -n plugin/langchain/lib/filesystem/agentfs/agentfs.ts | head -300Repository: eggjs/tegg
Length of output: 11615
🏁 Script executed:
# Also search for any Date conversions or timestamp handling in agentfs
rg -n "mtime|ctime|Date\.now\|getTime\|seconds" plugin/langchain/lib/filesystem/agentfs/agentfs.tsRepository: eggjs/tegg
Length of output: 1287
🏁 Script executed:
# Look for the FileInfo interface definition
rg -n "interface FileInfo\|type FileInfo" plugin/langchain/lib/filesystem/ -A 10Repository: eggjs/tegg
Length of output: 36
🏁 Script executed:
# Read the readRaw method from MysqlFilesystem.ts
cat -n plugin/langchain/lib/filesystem/MysqlFilesystem.ts | sed -n '321,345p'Repository: eggjs/tegg
Length of output: 1096
Multiply timestamps by 1000 to convert from seconds to milliseconds.
AgentFS stores times in seconds (line 109 of agentfs.ts: Date.now() / 1000), but new Date() expects milliseconds. This causes incorrect timestamps throughout the code. Fix by multiplying by 1000 in lsInfo() (lines 159, 166, 189, 196) and readRaw() (lines 342, 343).
Proposed fix
- modified_at: new Date(entryStat.mtime).toString(),
+ modified_at: new Date(entryStat.mtime * 1000).toString(),- created_at: new Date(stat.ctime).toString(),
- modified_at: new Date(stat.mtime).toString(),
+ created_at: new Date(stat.ctime * 1000).toString(),
+ modified_at: new Date(stat.mtime * 1000).toString(),🤖 Prompt for AI Agents
In `@plugin/langchain/lib/filesystem/MysqlFilesystem.ts` around lines 155 - 167,
The timestamps being passed into new Date() in MysqlFilesystem.ts are in seconds
(AgentFS uses seconds), so update the timestamp conversions in the lsInfo() and
readRaw() methods to multiply the stored time values by 1000 before creating
Date objects; e.g., change new Date(entryStat.mtime) to new Date(entryStat.mtime
* 1000) (apply the same change for all occurrences in lsInfo() and readRaw(),
and for any other places in MysqlFilesystem.ts where AgentFS-style epoch seconds
are converted to Date).
| let regex: RegExp; | ||
| try { | ||
| regex = new RegExp(pattern); | ||
| } catch (e: any) { | ||
| return `Invalid regex pattern: ${e.message}`; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the relevant file section
cat -n plugin/langchain/lib/filesystem/MysqlFilesystem.ts | sed -n '350,375p'Repository: eggjs/tegg
Length of output: 973
🏁 Script executed:
# Check what function contains this code and how pattern is passed
cat -n plugin/langchain/lib/filesystem/MysqlFilesystem.ts | sed -n '340,380p'Repository: eggjs/tegg
Length of output: 1444
🏁 Script executed:
# Search for where grepMatchesFromFiles is called to understand pattern source
rg -n "grepMatchesFromFiles" plugin/langchain/lib/filesystem/MysqlFilesystem.ts -B 5 -A 5Repository: eggjs/tegg
Length of output: 520
🏁 Script executed:
# Check if safe-regex2 is already used in the codebase
rg -r "safe-regex" --type ts --type jsRepository: eggjs/tegg
Length of output: 450
🏁 Script executed:
# Check if there are existing regex validation patterns in the codebase
rg -r "RegExp|new RegExp" plugin/langchain/lib/ --type ts -A 2 -B 2 | head -50Repository: eggjs/tegg
Length of output: 36
🏁 Script executed:
# Check package.json for dependencies
cat package.json | grep -A 30 '"dependencies"'Repository: eggjs/tegg
Length of output: 36
🏁 Script executed:
# Find where grepRaw is called
rg "grepRaw" --type ts --type js -B 3 -A 3Repository: eggjs/tegg
Length of output: 2369
🏁 Script executed:
# Check the MysqlFilesystem class context and its usage
rg "MysqlFilesystem" --type ts --type js -B 2 -A 2Repository: eggjs/tegg
Length of output: 3935
🏁 Script executed:
# Look for agent or LLM-related context
rg -l "agentFs\|Agent\|LLM" plugin/langchain/lib/filesystem/ --type tsRepository: eggjs/tegg
Length of output: 36
🏁 Script executed:
# Check the full class definition to understand its purpose
head -50 plugin/langchain/lib/filesystem/MysqlFilesystem.tsRepository: eggjs/tegg
Length of output: 2087
🏁 Script executed:
# Check if there's a tools or actions export that uses this
rg "export.*Filesystem\|tools\|actions" plugin/langchain/lib/ --type ts | head -20Repository: eggjs/tegg
Length of output: 36
Guard against ReDoS when compiling user-provided regex patterns.
new RegExp(pattern) receives untrusted input from LLM agents and can hang the event loop with catastrophic backtracking patterns. The try-catch only catches syntax errors, not ReDoS. Add a safety check (e.g., safe-regex2) or strict length/complexity limits before compiling.
🔒 Proposed mitigation (example using safe-regex2)
+import safeRegex from 'safe-regex2';
...
let regex: RegExp;
try {
+ if (!safeRegex(pattern)) {
+ return 'Invalid regex pattern: potentially unsafe';
+ }
regex = new RegExp(pattern);
} catch (e: any) {
return `Invalid regex pattern: ${e.message}`;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let regex: RegExp; | |
| try { | |
| regex = new RegExp(pattern); | |
| } catch (e: any) { | |
| return `Invalid regex pattern: ${e.message}`; | |
| } | |
| import safeRegex from 'safe-regex2'; |
| let regex: RegExp; | |
| try { | |
| regex = new RegExp(pattern); | |
| } catch (e: any) { | |
| return `Invalid regex pattern: ${e.message}`; | |
| } | |
| let regex: RegExp; | |
| try { | |
| if (!safeRegex(pattern)) { | |
| return 'Invalid regex pattern: potentially unsafe'; | |
| } | |
| regex = new RegExp(pattern); | |
| } catch (e: any) { | |
| return `Invalid regex pattern: ${e.message}`; | |
| } |
🧰 Tools
🪛 ast-grep (0.40.5)
[warning] 361-361: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(pattern)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
In `@plugin/langchain/lib/filesystem/MysqlFilesystem.ts` around lines 360 - 365,
The code currently calls new RegExp(pattern) directly (seen in
MysqlFilesystem.ts where variables pattern and regex are used), which is
vulnerable to ReDoS from untrusted LLM input; before compiling, validate and
sanitize the pattern by either using a safe-regex library (e.g., safe-regex2) to
test the pattern for catastrophic backtracking, or enforce strict limits (max
length and reject nested quantifiers/complex constructs) and only then call new
RegExp; if the pattern fails the safety check, return a clear error string
similar to the existing Invalid regex response instead of compiling.
| export function getFileSystemConfig(config: ModuleConfig, objectInfo: ObjectInfo): ConfigTypeHelper<'filesystem'> { | ||
| const filesystemName = objectInfo.qualifiers.find(t => t.attribute === FileSystemQualifierAttribute)?.value; | ||
| assert(filesystemName, 'not found filesystem name'); | ||
| const fsConfig = config.filesystem?.clients[filesystemName]; | ||
| if (!config) { | ||
| throw new Error(`not found FsMiddleware config for ${filesystemName}`); | ||
| } | ||
| return fsConfig!; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix missing config guard in getFileSystemConfig.
if (!config) will never fire and fsConfig! can be undefined, causing downstream failures. Guard the resolved client instead.
🐛 Proposed fix
export function getFileSystemConfig(config: ModuleConfig, objectInfo: ObjectInfo): ConfigTypeHelper<'filesystem'> {
const filesystemName = objectInfo.qualifiers.find(t => t.attribute === FileSystemQualifierAttribute)?.value;
assert(filesystemName, 'not found filesystem name');
const fsConfig = config.filesystem?.clients[filesystemName];
- if (!config) {
- throw new Error(`not found FsMiddleware config for ${filesystemName}`);
- }
- return fsConfig!;
+ if (!fsConfig) {
+ throw new Error(`not found filesystem config for ${filesystemName}`);
+ }
+ return fsConfig;
}🤖 Prompt for AI Agents
In `@plugin/langchain/lib/util.ts` around lines 27 - 34, getFileSystemConfig
currently checks `if (!config)` which is incorrect and allows `fsConfig` (from
`config.filesystem?.clients[filesystemName]`) to be undefined; update the guard
to validate `fsConfig` instead of `config` and throw a clear error referencing
`filesystemName` if the client is missing, ensuring the function does not return
`undefined` for `fsConfig`. Use the existing `filesystemName` lookup and
`config.filesystem?.clients` to locate the client and replace the faulty `if
(!config)` block with a check like `if (!fsConfig) throw new Error(...)`,
removing the unsafe non-null assertion (`fsConfig!`) thereafter.
| export const FileSystemConfigModuleConfigSchema = Type.Object({ | ||
| clients: Type.Record(Type.String(), Type.Object({ | ||
| dataSource: Type.String({ | ||
| description: 'The name of the data source to use for the MySQL filesystem middleware.', | ||
| }), | ||
| cwd: Type.Optional(Type.String({ | ||
| description: 'The current working directory for the filesystem middleware. Defaults to "/"', | ||
| })), | ||
| virtualMode: Type.Optional(Type.Boolean({ | ||
| description: 'Whether to enable virtual mode, which restricts file operations to within the specified cwd.', | ||
| })), | ||
| })), | ||
| }, { | ||
| title: 'FsMiddleware 设置', | ||
| name: 'FsMiddleware', | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
find . -name "index.d.ts" -path "*/plugin/langchain/typings/*" | head -5Repository: eggjs/tegg
Length of output: 91
🏁 Script executed:
git ls-files | grep -E "plugin/langchain/typings"Repository: eggjs/tegg
Length of output: 89
🏁 Script executed:
cat -n plugin/langchain/typings/index.d.ts | head -150Repository: eggjs/tegg
Length of output: 8114
🏁 Script executed:
# Search for FileSystemConfigModuleConfigSchema usage
rg "FileSystemConfigModuleConfigSchema" --type ts --type tsx -B 2 -A 2Repository: eggjs/tegg
Length of output: 81
🏁 Script executed:
# Search for how config keys/schemas are registered in tegg
rg "ModuleConfig|ConfigModuleConfigSchema" --type ts --type tsx | grep -i "filesystem\|fsMiddleware" -A 1 -B 1Repository: eggjs/tegg
Length of output: 81
🏁 Script executed:
rg "filesystem" -A 3 -B 3 | head -100Repository: eggjs/tegg
Length of output: 6757
🏁 Script executed:
rg "FsMiddleware" -A 2 -B 2 | head -100Repository: eggjs/tegg
Length of output: 1133
🏁 Script executed:
# Search for how tegg uses schema name/title properties in config validation
rg "name.*title" --type ts | grep -i "schema\|config" | head -20Repository: eggjs/tegg
Length of output: 36
🏁 Script executed:
# Look for plugin registration or module config setup
fd "plugin" --type d | head -10Repository: eggjs/tegg
Length of output: 235
🏁 Script executed:
# Check if there's a module definition or config export for this plugin
find . -name "*module*" -o -name "*config*" | grep -i "langchain\|filesystem" | head -10Repository: eggjs/tegg
Length of output: 594
🏁 Script executed:
# Look at the config and module files to understand how schemas are mapped
cat plugin/langchain/test/fixtures/apps/langchain/config/module.jsonRepository: eggjs/tegg
Length of output: 182
🏁 Script executed:
# Check if there's any config setup that shows how the schema is registered
cat plugin/langchain/lib/config/config.default.ts 2>/dev/null || find plugin/langchain -name "*config*" -type f | grep -v node_modules | head -5Repository: eggjs/tegg
Length of output: 250
🏁 Script executed:
# Search for how tegg processes schema name/title in validation
rg "schema.*name\|name.*schema" --type ts | grep -i "register\|validate\|config" | head -20Repository: eggjs/tegg
Length of output: 36
🏁 Script executed:
# Check the module.yml or other plugin entry points
find plugin/langchain -name "module.yml" -o -name "module.json" -o -name "plugin.ts" | xargs cat 2>/dev/nullRepository: eggjs/tegg
Length of output: 836
🏁 Script executed:
# Compare ChatModel and FileSystem patterns in typings
sed -n '106,109p' plugin/langchain/typings/index.d.ts
sed -n '125,128p' plugin/langchain/typings/index.d.ts
sed -n '132,140p' plugin/langchain/typings/index.d.tsRepository: eggjs/tegg
Length of output: 416
🏁 Script executed:
# Search tegg core for how schema name/title is used in validation
rg "schema.*name|ConfigSchema" --type ts core/ | grep -i "validate\|register" | head -15Repository: eggjs/tegg
Length of output: 36
🏁 Script executed:
# Check if tegg-langchain-decorator or tegg core uses the schema name for validation
find core -name "*config*" -o -name "*schema*" | grep -E "\.(ts|js)$" | head -10Repository: eggjs/tegg
Length of output: 36
🏁 Script executed:
# Look for tegg config validation implementation
rg "ModuleConfig.*Schema\|name.*ChatModel\|name.*filesystem" --type ts | head -20Repository: eggjs/tegg
Length of output: 36
🏁 Script executed:
# Check documentation or comments about schema name usage
rg "Type.Object.*name\|title.*name" --type ts -B 5 -A 1 | head -30Repository: eggjs/tegg
Length of output: 36
🏁 Script executed:
# See if there's tegg documentation about schema validation
find . -name "*.md" | xargs grep -l "schema.*name\|ConfigSchema" 2>/dev/null | head -5Repository: eggjs/tegg
Length of output: 311
🏁 Script executed:
# Look for how tegg validates module config against schemas
rg "validateSync\|validate.*config\|ajv" --type ts core/ | grep -i schema | head -15Repository: eggjs/tegg
Length of output: 36
🏁 Script executed:
# Check if the schema name field is actually used anywhere in tegg
rg "\.name" --type ts core/ | grep -i "schema\|config" | grep -v "fileName\|moduleName\|objectName" | head -20Repository: eggjs/tegg
Length of output: 2172
🏁 Script executed:
# Search for how schemas are actually used in validation - look for ajv usage
rg "ajv.*validate\|schema.*validate" --type ts core/ | head -20Repository: eggjs/tegg
Length of output: 36
🏁 Script executed:
# Check if tegg config validation uses the schema name or just the structure
rg "ModuleConfig\|ConfigSchema" --type ts core/runtime/src -A 3 | head -40Repository: eggjs/tegg
Length of output: 36
🏁 Script executed:
# Look for error messages that reference schema names
rg "FsMiddleware\|ChatModel" --type ts | grep -i error | head -10Repository: eggjs/tegg
Length of output: 36
🏁 Script executed:
# Check the pattern - see if name matches config keys in other schemas
rg "export const.*Schema.*Type.Object" --type ts -A 5 | grep -E "name:|title:" | head -20Repository: eggjs/tegg
Length of output: 36
🏁 Script executed:
# Search broader for where schema name/title is used
rg "name.*FsMiddleware\|FsMiddleware.*name" -A 3 -B 3Repository: eggjs/tegg
Length of output: 36
🏁 Script executed:
# Check if there's a pattern in how tegg consumes schema metadata
rg "schema.*\\.name\|schema\\[.*name" --type ts core/ | head -20Repository: eggjs/tegg
Length of output: 36
🏁 Script executed:
# Verify the actual config structure and how it's validated against schemas
rg "ChatModel\|filesystem" --type ts plugin/langchain/lib -A 2 -B 2 | grep -E "config\.|schema" | head -30Repository: eggjs/tegg
Length of output: 36
🏁 Script executed:
# Check if the schema name/title is simply for documentation or actually used in validation
cat plugin/langchain/lib/config/config.default.js 2>/dev/null || find plugin/langchain -name "*.js" -path "*/config/*" -exec cat {} \;Repository: eggjs/tegg
Length of output: 974
🏁 Script executed:
# Check if schema name is used in error messages or logging
rg "FsMiddleware\|ChatModel" --type ts plugin/langchain/lib | grep -i "error\|message\|log"Repository: eggjs/tegg
Length of output: 36
🏁 Script executed:
# Verify the naming pattern by checking all schema definitions
rg "export const.*ConfigModuleConfigSchema" --type ts -A 6 | grep -E "name:|title:|export const"Repository: eggjs/tegg
Length of output: 252
🏁 Script executed:
# Check if there's documentation about schema name field purpose
rg "Type.Object.*\{.*name\|name.*:" --type ts -B 5 | grep -i "description\|comment\|doc" | head -10Repository: eggjs/tegg
Length of output: 36
🏁 Script executed:
# Check if there are other modules/schemas to see the naming pattern
rg "ConfigModuleConfigSchema.*Type.Object" --type ts -A 6 | grep -E "name:|title:" -B 1Repository: eggjs/tegg
Length of output: 36
🏁 Script executed:
# Search for any tegg documentation about schema structure
find . -name "*.md" -path "*/core/*" | xargs grep -l "schema\|ConfigSchema" 2>/dev/null | head -5Repository: eggjs/tegg
Length of output: 36
🏁 Script executed:
# Check the `@eggjs/tegg/ajv` package to understand schema usage
find . -path "*/node_modules/@eggjs/tegg/ajv*" -o -path "*/core/*ajv*" | grep -E "\.(ts|d.ts)$" | head -10Repository: eggjs/tegg
Length of output: 289
🏁 Script executed:
# Check the tegg ajv exports and how schemas are used
cat core/tegg/ajv.tsRepository: eggjs/tegg
Length of output: 91
🏁 Script executed:
# Check how AJV schemas are validated in tegg
rg "validateSync\|validate" --type ts core/ajv-decorator/src | head -20Repository: eggjs/tegg
Length of output: 36
🏁 Script executed:
# Look for how config schemas are actually applied in tegg-config plugin
find . -path "*tegg-config*" -name "*.ts" | xargs cat 2>/dev/null | head -100Repository: eggjs/tegg
Length of output: 36
🏁 Script executed:
# Search for how tegg-config plugin validates configs with schemas
rg "ChatModelConfigModuleConfigSchema\|FileSystemConfigModuleConfigSchema" --type ts -A 5 -B 5Repository: eggjs/tegg
Length of output: 36
🏁 Script executed:
# Look in tegg-config plugin source
find . -name "*tegg-config*" -type d | head -5Repository: eggjs/tegg
Length of output: 36
🏁 Script executed:
# Check if schema name is used in error messages or validation
rg "ConfigModuleConfigSchema" --type ts core/ | head -20Repository: eggjs/tegg
Length of output: 36
🏁 Script executed:
# Final verification: confirm the config key vs schema name mapping
sed -n '113,140p' plugin/langchain/typings/index.d.tsRepository: eggjs/tegg
Length of output: 1031
🏁 Script executed:
# Verify ChatModel follows the same pattern
sed -n '8,111p' plugin/langchain/typings/index.d.ts | grep -E "ConfigModuleConfigSchema|name:|title:" -A 1 -B 1Repository: eggjs/tegg
Length of output: 323
Align schema name with config key (filesystem) for consistency with ChatModel pattern.
The ChatModel schema uses name: 'ChatModel' matching its config key, but FileSystemConfigModuleConfigSchema uses name: 'FsMiddleware' while the config key is filesystem. This naming inconsistency causes mismatch in error messages (which reference FsMiddleware) and configuration documentation. Align to match the ChatModel pattern.
Suggested alignment
}, {
- title: 'FsMiddleware 设置',
- name: 'FsMiddleware',
+ title: 'filesystem 设置',
+ name: 'filesystem',
});🤖 Prompt for AI Agents
In `@plugin/langchain/typings/index.d.ts` around lines 113 - 128, The schema
FileSystemConfigModuleConfigSchema currently sets the schema option name to
'FsMiddleware', causing mismatch with the config key 'filesystem'; update the
Type.Object options for FileSystemConfigModuleConfigSchema to use name:
'filesystem' (so error messages and docs reference the config key consistently,
mirroring the ChatModel pattern) and leave the rest of the schema unchanged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@plugin/langchain/test/llm.test.ts`:
- Around line 512-521: The two tests labeled "should resolve when a file exists"
and "should resolve when a directory exists" never call fs.access() or assert
its behavior; update each test to call await fs.access('/exists.txt') and await
fs.access('/existsdir') respectively (or use an assertion that the promise
resolves) after creating the file/dir, and then perform the existing cleanup
(fs.rm) so the test actually verifies access() succeeds; reference the two test
blocks by their descriptions to locate and modify them.
- Line 944: The test currently calls assert.rejects without awaiting it, so the
assertion is never evaluated; update the test to await the assertion and pass
the promise returned by mysql.query rather than a non-awaited function wrapper —
i.e., add await before assert.rejects and call assert.rejects with the promise
from mysql.query(insertSql, [], { executeType: 'execute' }) (referencing
assert.rejects, mysql.query, and insertSql) so the test correctly fails on
non-rejection.
- Line 914: The assertion uses a string literal for the COUNT result which is
inconsistent with other tests and brittle across MySQL driver return types;
update the assertion to compare numerically by wrapping the value with
Number(result!.count) (i.e., change the assert.deepStrictEqual call that
compares result!.count to use Number(result!.count) and compare to 3) so it
matches other checks and handles number/string/BigInt returns consistently.
♻️ Duplicate comments (1)
plugin/langchain/test/llm.test.ts (1)
44-53: Consider consolidating table cleanup into a loop.The database cleanup logic is repetitive. Iterating over a list of table names would be more maintainable.
🧹 Nitpick comments (3)
plugin/langchain/test/llm.test.ts (3)
605-639: Remove commented-out code.The function contains commented-out SQL statements that add noise and reduce readability. Since these are unused, they should be removed.
♻️ Suggested cleanup
async function getChunkCount(path: string): Promise<number> { - // const stmt = mysql.prepare(` - // SELECT COUNT(*) as count FROM fs_data - // WHERE ino = (SELECT ino FROM fs_dentry WHERE parent_ino = 1 AND name = ?) - // `); - // const sql = SqlString.format(` - // SELECT COUNT(*) as count FROM fs_data - // WHERE ino = (SELECT ino FROM fs_dentry WHERE parent_ino = 1 AND name = ?) - // `, [ path ]); const pathParts = path.split('/').filter(p => p); - // const name = pathParts[pathParts.length - 1]; - - // For simple paths, get the inode from the path - // const lookupStmt = db.prepare(` - // SELECT d.ino FROM fs_dentry d WHERE d.parent_ino = ? AND d.name = ? - // `); let currentIno = 1; // root
778-793: Remove commented-out helper function.The
getInofunction is entirely commented out. Dead code should be removed; version control preserves history if needed later.
868-885: Missing cleanup for large file test.Unlike other tests in this suite, this test doesn't clean up
/large.binafter completion. While the outerafter()hook will eventually clear the tables, adding explicit cleanup maintains consistency with other tests.♻️ Proposed fix
const actualChunks = await getChunkCount('/large.bin'); assert.deepStrictEqual(actualChunks, expectedChunks); + await fs.rm('/large.bin'); });
| // access() Operations | ||
| it('should resolve when a file exists', async () => { | ||
| await fs.writeFile('/exists.txt', 'content'); | ||
| await fs.rm('/exists.txt'); | ||
| }); | ||
|
|
||
| it('should resolve when a directory exists', async () => { | ||
| await fs.mkdir('/existsdir'); | ||
| await fs.rm('/existsdir', { recursive: true }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests for access() don't actually call fs.access().
These tests are named for testing access() but only create and remove files/directories without actually invoking fs.access() to verify existence. The assertions are missing.
🐛 Proposed fix
it('should resolve when a file exists', async () => {
await fs.writeFile('/exists.txt', 'content');
+ await fs.access('/exists.txt'); // should not throw
await fs.rm('/exists.txt');
});
it('should resolve when a directory exists', async () => {
await fs.mkdir('/existsdir');
+ await fs.access('/existsdir'); // should not throw
await fs.rm('/existsdir', { recursive: true });
});🤖 Prompt for AI Agents
In `@plugin/langchain/test/llm.test.ts` around lines 512 - 521, The two tests
labeled "should resolve when a file exists" and "should resolve when a directory
exists" never call fs.access() or assert its behavior; update each test to call
await fs.access('/exists.txt') and await fs.access('/existsdir') respectively
(or use an assertion that the promise resolves) after creating the file/dir, and
then perform the existing cleanup (fs.rm) so the test actually verifies access()
succeeds; reference the two test blocks by their descriptions to locate and
modify them.
| [ currentIno ], | ||
| ); | ||
| const result = (await mysql.query(countStmt, [], { executeType: 'execute' }) as { count: number }[])[0]; | ||
| assert.deepStrictEqual(result!.count, '3'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent type comparison for COUNT result.
This assertion compares result!.count to the string '3', while similar assertions elsewhere (e.g., lines 1000, 1008, 1050) use Number(result.count) for numeric comparison. MySQL drivers may return COUNT as number, string, or BigInt depending on configuration. Use consistent handling.
🐛 Proposed fix
- assert.deepStrictEqual(result!.count, '3');
+ assert.deepStrictEqual(Number(result!.count), 3);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert.deepStrictEqual(result!.count, '3'); | |
| assert.deepStrictEqual(Number(result!.count), 3); |
🤖 Prompt for AI Agents
In `@plugin/langchain/test/llm.test.ts` at line 914, The assertion uses a string
literal for the COUNT result which is inconsistent with other tests and brittle
across MySQL driver return types; update the assertion to compare numerically by
wrapping the value with Number(result!.count) (i.e., change the
assert.deepStrictEqual call that compares result!.count to use
Number(result!.count) and compare to 3) so it matches other checks and handles
number/string/BigInt returns consistently.
| 'INSERT INTO fs_data (ino, chunk_index, data) VALUES (?, ?, ?)', | ||
| [ ino, 0, Buffer.from([ 1, 2, 3 ]) ], | ||
| ); | ||
| assert.rejects(() => mysql.query(insertSql, [], { executeType: 'execute' })); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing await on assert.rejects will cause test to always pass.
assert.rejects returns a Promise that must be awaited. Without await, the test completes before the assertion is evaluated, causing it to pass regardless of whether the duplicate insert actually throws.
🐛 Proposed fix
- assert.rejects(() => mysql.query(insertSql, [], { executeType: 'execute' }));
+ await assert.rejects(() => mysql.query(insertSql, [], { executeType: 'execute' }));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert.rejects(() => mysql.query(insertSql, [], { executeType: 'execute' })); | |
| await assert.rejects(() => mysql.query(insertSql, [], { executeType: 'execute' })); |
🤖 Prompt for AI Agents
In `@plugin/langchain/test/llm.test.ts` at line 944, The test currently calls
assert.rejects without awaiting it, so the assertion is never evaluated; update
the test to await the assertion and pass the promise returned by mysql.query
rather than a non-awaited function wrapper — i.e., add await before
assert.rejects and call assert.rejects with the promise from
mysql.query(insertSql, [], { executeType: 'execute' }) (referencing
assert.rejects, mysql.query, and insertSql) so the test correctly fails on
non-rejection.
33ca70a to
766898b
Compare
766898b to
fe938fa
Compare
| // } finally { | ||
| // await fd.close(); | ||
| // } | ||
| content = await this.agentFs.readFile(resolvedPath, 'utf-8'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
offset 和 limit 需要传到下层,存储的文件可能是 MB 级别的。会造成 db 性能很差。
| path?: string | null, | ||
| glob?: string | null, | ||
| ): Promise<GrepMatch[] | string> { | ||
| const files = await this.agentFs.readallFiles(this.cwd); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里也会导致内存爆炸。
| await this.agentFs.writeFile(resolvedPath, newContent, 'utf-8'); | ||
|
|
||
| return { path: filePath, filesUpdate: null, occurrences }; | ||
| } catch (e: any) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
需要加日志来追踪一下调用栈。
| await this.db.query(` | ||
| CREATE TABLE IF NOT EXISTS fs_config ( | ||
| id INTEGER AUTO_INCREMENT PRIMARY KEY, | ||
| \`key\` TEXT NOT NULL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
不要用关键字。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
注释。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ctime 和 mtime 的字段需要支持配置。
| const sql = SqlString.format(` | ||
| SELECT chunk_index, data FROM fs_data | ||
| WHERE ino = ? AND chunk_index >= ? AND chunk_index <= ? | ||
| ORDER BY chunk_index ASC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
limit 限制下?防止查询量太大。
| } | ||
|
|
||
| const sql = SqlString.format( | ||
| 'SELECT size FROM fs_inode WHERE ino = ?', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
参考 dal 的方式 sql 单独维护在一个模版文件里方便 review。
| await this.writeDataAtOffset(offset, data); | ||
|
|
||
| const newSize = Math.max(currentSize, offset + data.length); | ||
| const now = Math.floor(Date.now() / 1000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
直接用 now()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
建表的时候设置好 update
| const upsertSql = SqlString.format(` | ||
| INSERT INTO fs_data (ino, chunk_index, data) VALUES (?, ?, ?) | ||
| ON CONFLICT(ino, chunk_index) DO UPDATE SET data = excluded.data | ||
| `, [ this.ino, chunkIdx, chunkData ]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
冲突的时候应该直接报错,让上层处理。
| 'SELECT size FROM fs_inode WHERE ino = ?', | ||
| [ this.ino ], | ||
| ); | ||
| const sizeRow = (await this.db.query(sql) as { size: number }[])[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
可能不存在?
|
|
||
| const now = Math.floor(Date.now() / 1000); | ||
| const updateSql = SqlString.format( | ||
| 'UPDATE fs_inode SET size = ?, mtime = ? WHERE ino = ?', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now()
| await this.db.query('PRAGMA synchronous = FULL', [], { executeType: 'execute' }); | ||
| await this.db.query('PRAGMA wal_checkpoint(TRUNCATE)', [], { executeType: 'execute' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
db 不需要 sync,没有 buffer。
| private async initialize(): Promise<void> { | ||
| await this.db.query(` | ||
| CREATE TABLE IF NOT EXISTS fs_config ( | ||
| id INTEGER AUTO_INCREMENT PRIMARY KEY, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
long
| `, [], { executeType: 'execute' }); | ||
|
|
||
| await this.db.query(` | ||
| CREATE TABLE IF NOT EXISTS fs_symlink ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
是否支持 hardlink?
| /** | ||
| * A filesystem backed by SQLite, implementing the FileSystem interface. | ||
| */ | ||
| export class AgentFS implements FileSystem { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
如何会话隔离?
killagu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
由于 fs 大小无法确定,可能会造成 mysql 非常大行数的读取,以及应用内存的压力。建议标记为试用,勿在生产使用。
生产推荐基于 oss 这类实现。
Checklist
npm testpassesAffected core subsystem(s)
Description of change
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.