feat(sil): Automated improvements from self-improvement loop#155
feat(sil): Automated improvements from self-improvement loop#155glassBead-tc wants to merge 2 commits intomainfrom
Conversation
Run summary: - Total iterations: 4 - Successful: 3 - Failed: 0 Generated by Self-Improvement Loop workflow.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
🤖 Augment PR SummarySummary: This PR applies automated changes from the Self‑Improvement Loop, focusing on hardening filesystem persistence, reducing promise-queue memory pressure, and improving type safety for operation catalogs. Changes:
Technical Notes: Main risk areas are cross-platform path handling (especially Windows path semantics) and correctness of the new queue-reset logic under concurrent requests. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| } | ||
|
|
||
| // Check for valid UUID format or safe alphanumeric pattern only | ||
| const validIdPattern = /^[a-zA-Z0-9-_]+$/; |
There was a problem hiding this comment.
validIdPattern uses an unescaped - inside the character class ([0-9-_]), which in JS regex forms a range and can unintentionally allow characters like :/@/[ etc. That can undermine the path-traversal defense (e.g., drive-letter style paths on Windows); same issue exists in validPartitionPattern.
Severity: high
Other Locations
src/persistence/filesystem-storage.ts:87
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| actionResult: validatedInput.actionResult, | ||
| beliefs: validatedInput.beliefs, | ||
| assumptionChange: validatedInput.assumptionChange, | ||
| contextData: validatedInput.contextData, | ||
| }; |
There was a problem hiding this comment.
Resetting this.processingQueue after await this.processingQueue can overwrite a newer queue created by a concurrent processThought() call, which would allow operations to run concurrently and reintroduce race conditions. The same “reset the queue reference later” pattern in the reasoning channel’s updateQueue looks like it can also break serialization.
Severity: high
Other Locations
src/observatory/channels/reasoning.ts:57
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
Greptile SummaryCritical Issues FoundThis PR applies three automated self-improvement iterations to the codebase with generally sound goals (security hardening, memory pressure relief, type safety), but introduces regressions that break core functionality: 1. Critical: Loops Catalog Emptied (
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private queueUpdate<T>(operation: () => T | Promise<T>): Promise<T> { | ||
| const queued = this.updateQueue.then(() => operation()); | ||
| this.updateQueue = queued.then(() => {}, () => {}); // Continue queue even on error | ||
| // Increment counter and reset queue periodically to prevent memory pressure | ||
| this.queueResetCounter++; | ||
| if (this.queueResetCounter >= this.QUEUE_RESET_INTERVAL) { | ||
| this.queueResetCounter = 0; | ||
| // Wait for current queue to complete, then reset | ||
| this.updateQueue.then(() => {}, () => {}).finally(() => { | ||
| this.updateQueue = Promise.resolve(); |
There was a problem hiding this comment.
Async .finally() reset creates queue serialization race condition
The .finally() callback used to reset this.updateQueue fires asynchronously — after the current queue promise resolves. By that time, the code immediately after it has already chained the new operation and updated the queue:
// Time T (synchronous):
this.updateQueue.then(…).finally(() => {
this.updateQueue = Promise.resolve(); // fires LATER at T+δ
});
const queued = this.updateQueue.then(…); // chains to current queue NOW
this.updateQueue = queued.then(…); // sets to Q
// Time T+δ (async, when old queue resolves):
// .finally fires and overwrites this.updateQueue = Promise.resolve()
// This DISCARDS the reference to Q!
Any operation arriving between T and T+δ is correctly chained to Q. But at T+δ, the .finally() overwrites this.updateQueue with Promise.resolve(). Subsequent operations then chain to Promise.resolve() instead of Q — running concurrently with operations still executing from Q. This breaks the serialization guarantee.
A safer approach: replace the .finally() reset with synchronous queue-chaining that only resets when already settled:
private queueUpdate<T>(operation: () => T | Promise<T>): Promise<T> {
const queued = this.updateQueue.then(() => operation());
this.updateQueue = queued.then(() => {}, () => {}); // Continue queue even on error
return queued;
}The 9 test failures noted in iteration-2's evaluation ("branch retrieval session setup issues") may be a symptom of this broken ordering.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/observatory/channels/reasoning.ts
Line: 51-58
Comment:
**Async `.finally()` reset creates queue serialization race condition**
The `.finally()` callback used to reset `this.updateQueue` fires asynchronously — after the current queue promise resolves. By that time, the code immediately after it has already chained the new operation and updated the queue:
```
// Time T (synchronous):
this.updateQueue.then(…).finally(() => {
this.updateQueue = Promise.resolve(); // fires LATER at T+δ
});
const queued = this.updateQueue.then(…); // chains to current queue NOW
this.updateQueue = queued.then(…); // sets to Q
// Time T+δ (async, when old queue resolves):
// .finally fires and overwrites this.updateQueue = Promise.resolve()
// This DISCARDS the reference to Q!
```
Any operation arriving between T and T+δ is correctly chained to Q. But at T+δ, the `.finally()` overwrites `this.updateQueue` with `Promise.resolve()`. Subsequent operations then chain to `Promise.resolve()` instead of Q — running concurrently with operations still executing from Q. This breaks the serialization guarantee.
A safer approach: replace the `.finally()` reset with synchronous queue-chaining that only resets when already settled:
```typescript
private queueUpdate<T>(operation: () => T | Promise<T>): Promise<T> {
const queued = this.updateQueue.then(() => operation());
this.updateQueue = queued.then(() => {}, () => {}); // Continue queue even on error
return queued;
}
```
The 9 test failures noted in iteration-2's evaluation ("branch retrieval session setup issues") may be a symptom of this broken ordering.
How can I resolve this? If you propose a fix, please make it concise.1 participant
Run summary:
Generated by Self-Improvement Loop workflow.