Feature/ragdiary default config selfcheck - #1
Conversation
- 新增同 hostId 普通 SSH 命令 FIFO 队列、排队超时和队列长度限制 - SSH 命令超时后隔离脏连接，并在代理 RPC 超时预算中计入排队时间 - LinuxShellExecutor 分流 tail/journalctl 长待机任务到日志监控，并前置拦截 sudo/su/pkexec/doas - README 记录连接池持久化相关修订，.gitignore 忽略 helloagents/ Smoke: node --check 相关 JS 文件；mock 覆盖日志分流、提权拦截、执行队列、超时断连和代理超时预算
…ool-persistence 调整sshmanager日志输出、以及加固ssh命令执行顺序
VCPTavern 预设注入占位符添加 `::blacklist` 语法
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a9dd6e1b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const queueWaitTimeout = Number.parseInt(options.queueWaitTimeout, 10); | ||
| const queueBudget = Number.isFinite(queueWaitTimeout) && queueWaitTimeout > 0 | ||
| ? queueWaitTimeout | ||
| : 120000; |
There was a problem hiding this comment.
When a caller sets queueWaitTimeout: 0, _enqueueHostExecution treats that as unlimited waiting, but the proxy substitutes a 120-second budget here and times out the RPC after roughly 150 seconds. The queued request is not cancelled on the service, so it can execute later after the caller has already received a failure, which is especially dangerous for state-changing shell commands. Preserve the unlimited setting or add cancellation so the proxy and service agree.
Useful? React with 👍 / 👎.
| if (logFollowCommand?.type === 'tail') { | ||
| return this._startLogFileMonitor(hostId, logFollowCommand.logPath, command, { |
There was a problem hiding this comment.
For commands such as tail -f app.log | grep ERROR or tail -f a.log b.log, the classifier still returns a single logPath, causing this branch to skip execution of the original command and monitor only the selected source file. This silently drops pipelines, filters, and all but the last input file. Only route a plain, single-file tail -f command here; compound commands should continue through the background-command path.
Useful? React with 👍 / 👎.
| const patterns = [ | ||
| { name: 'sudo', regex: /(^|[\s;&|()])sudo(\s|$)/ }, | ||
| { name: 'pkexec', regex: /(^|[\s;&|()])pkexec(\s|$)/ }, | ||
| { name: 'doas', regex: /(^|[\s;&|()])doas(\s|$)/ }, | ||
| { name: 'su', regex: /(^|[\s;&|()])su(\s+-|\s|$)/ } |
There was a problem hiding this comment.
These raw-text regexes also match harmless arguments and quoted text, so commands such as echo sudo, grep sudo audit.log, or printf 'use sudo carefully' are reported as privilege escalation and blocked before normal validation. Restrict detection to actual command positions using the shell parser/tokenizer rather than matching every whitespace-delimited occurrence.
Useful? React with 👍 / 👎.
| this._log(`SSH 命令超时，断开脏连接: ${connection.hostId}`); | ||
| if (connection.isPooled) { | ||
| this.disconnect(connection.hostId).catch(err => { |
There was a problem hiding this comment.
When pooling is enabled and a host already has a stream session, createStreamSession() reuses the host's pooled connection while ordinary commands can also reuse that same connection. If one ordinary command times out, this call disconnects the entire host connection, terminating unrelated tail -f or log-monitor sessions that are still active. Before disconnecting, avoid shared pooled connections with active stream sessions or isolate timed commands onto their own connection.
Useful? React with 👍 / 👎.
| async function buildRagDiaryConfigNotifications() { | ||
| const pluginNotifications = getRagDiaryPluginConfigNotifications(); | ||
| if (pluginNotifications.length > 0) { | ||
| return pluginNotifications; | ||
| } | ||
|
|
||
| return buildFallbackRagDiaryConfigNotifications(); |
There was a problem hiding this comment.
When RAGDiaryPlugin is disabled or absent, getRagDiaryPluginConfigNotifications() returns no notifications and this unconditional fallback inspects its files anyway. Every connecting VCPLog, VCPInfo, DistributedServer, and AdminPanel client can therefore receive warnings instructing users to configure a plugin they intentionally disabled, especially because its ignored runtime JSON files may be absent. Check that the plugin is registered before running the fallback.
Useful? React with 👍 / 👎.
| const isDefault = isSameJsonConfig(readResult.value, defaultValue); | ||
| return { | ||
| key: target.key, | ||
| label: target.label, | ||
| filePath, | ||
| value: readResult.value, | ||
| state: isDefault ? 'default' : 'custom', | ||
| message: isDefault ? `${target.label}配置仍为默认值` : `${target.label}配置已自定义` |
There was a problem hiding this comment.
Any syntactically valid JSON that differs from the default is classified as custom, so values such as null, [], or an unrelated object make the self-check report success and are also treated as eligible backups. For example, rag_tags.json containing null later fails at Object.keys(this.ragConfig), while this check silently labels and backs it up as a valid customization. Validate each target's required structure before assigning custom or updating its recovery backup.
Useful? React with 👍 / 👎.
| const shouldWriteBackup = !backupResult.ok || !isSameJsonConfig(backupResult.value, currentState.value); | ||
| if (shouldWriteBackup) { | ||
| await this._writeJsonConfigFileAtomic(backupPath, currentState.value); |
There was a problem hiding this comment.
In a read-only deployment, a valid custom configuration with no existing backup reaches this write because the backup JSON files are ignored and normally absent. The write error propagates out of _runConfigSelfCheck() and aborts loadConfig(), preventing RAGDiaryPlugin from initializing even though its actual configuration is readable. Treat backup creation as best-effort or store backups in a writable runtime-data directory.
Useful? React with 👍 / 👎.
| releaseOperation(); | ||
| } | ||
| rejectOnce(new Error(`SSH 命令执行超时 (${timeout}ms)`)); | ||
| const timeoutError = new Error(`SSH 命令执行超时 (${timeout}ms)`); | ||
| timeoutError.code = 'SSH_COMMAND_TIMEOUT'; | ||
| timeoutError.hostId = connection.hostId; | ||
| disconnectDirtyConnection(); | ||
| rejectOnce(timeoutError); |
There was a problem hiding this comment.
When disconnectOnCommandTimeout is explicitly false, the timeout releases the operation and rejects the queued task without stopping the remote command or disconnecting its pooled connection. _runHostExecution(...).finally() then starts the next queued command on that same host while the timed-out command may still be running, defeating the new per-host serialization guarantee and allowing state-changing commands to overlap. Hold the queue until the stream closes or isolate such commands from the shared connection.
Useful? React with 👍 / 👎.
|
|
||
| // 主机级命令执行队列：同一 hostId 的普通 SSH 命令默认串行执行。 | ||
| this.enableExecutionQueue = this.globalSettings.enableExecutionQueue !== false; | ||
| this.maxExecutionQueueLength = this.globalSettings.maxExecutionQueueLength || 50; |
There was a problem hiding this comment.
A configured maxExecutionQueueLength: 0 is replaced with 50 by this || fallback, even though _enqueueHostExecution() explicitly supports zero as the setting that rejects every queued command. Deployments using zero to prohibit waiting commands therefore unexpectedly accept up to 50 of them. Use a nullish/default check so the explicit zero value is retained.
Useful? React with 👍 / 👎.
2 participants
No description provided.