Compaction/Safeguard: add summary quality audit retries#25556
Compaction/Safeguard: add summary quality audit retries#25556jalehman merged 13 commits intoopenclaw:mainfrom
Conversation
| function extractOpaqueIdentifiers(text: string): string[] { | ||
| const matches = | ||
| text.match( | ||
| /([A-Fa-f0-9]{8,}|https?:\/\/\S+|\/[\w./-]+|[A-Za-z]:\\[\w\\.-]+|[A-Za-z0-9._-]+\.[A-Za-z0-9._/-]+:\d{1,5}|\b\d{6,}\b)/g, |
There was a problem hiding this comment.
regex pattern \/[\w./-]+ will match single-char paths like /a or /x, which are likely false positives
the pattern intends to match Unix-style paths but lacks a minimum length constraint; consider requiring at least 2 path segments or a minimum length to avoid matching single-letter paths that are unlikely to be meaningful identifiers
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/pi-extensions/compaction-safeguard.ts
Line: 294
Comment:
regex pattern `\/[\w./-]+` will match single-char paths like `/a` or `/x`, which are likely false positives
the pattern intends to match Unix-style paths but lacks a minimum length constraint; consider requiring at least 2 path segments or a minimum length to avoid matching single-letter paths that are unlikely to be meaningful identifiers
How can I resolve this? If you propose a fix, please make it concise.6db0640 to
8971441
Compare
8971441 to
94ab001
Compare
f073ec7 to
a35475c
Compare
a35475c to
d783ffb
Compare
|
This pull request has been automatically marked as stale due to inactivity. |
d783ffb to
b16cd70
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b16cd70531
ℹ️ 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 missingIdentifiers = params.identifiers.filter((id) => !params.summary.includes(id)); | ||
| if (missingIdentifiers.length > 0) { | ||
| reasons.push(`missing_identifiers:${missingIdentifiers.slice(0, 3).join(",")}`); |
There was a problem hiding this comment.
Respect identifier policy during quality audit
auditSummaryQuality always treats omitted identifiers as a failure and the retry path then tells the model to “include every required section with exact identifiers preserved,” which overrides the existing identifierPolicy behavior from resolveExactIdentifierSectionInstruction (including off and redaction-oriented custom policies). In sessions configured to avoid strict identifier retention, this can force sensitive IDs/tokens back into persisted compaction summaries and changes configured behavior rather than enforcing it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 183e644. The quality audit is now policy-aware: identifier-presence checks run only when identifierPolicy is strict, and retry feedback no longer forces strict retention when policy is off/custom.
Greptile SummaryThis PR introduces a bounded quality-audit and retry flow for compaction summaries, along with several hardening fixes across follow-up commits. When Key changes:
All previously raised review concerns (section substring vs. line match, preserved-turn identifier bypass, hex case-sensitivity, unsanitized reasons) have been addressed and verified in this branch. Confidence Score: 4/5
Last reviewed commit: 268b292 |
| const canRegenerate = | ||
| messagesToSummarize.length > 0 || | ||
| (preparation.isSplitTurn && turnPrefixMessages.length > 0); | ||
| if (!qualityGuardEnabled || !canRegenerate) { | ||
| summary = summaryWithPreservedTurns; | ||
| break; | ||
| } | ||
| const quality = auditSummaryQuality({ | ||
| summary: summaryWithoutPreservedTurns, | ||
| identifiers, | ||
| latestAsk: latestUserAsk, | ||
| identifierPolicy, | ||
| }); | ||
| const splitTurnSection = `**Turn Context (split turn):**\n\n${prefixSummary}`; | ||
| summary = historySummary.trim() | ||
| ? `${historySummary}\n\n---\n\n${splitTurnSection}` | ||
| : splitTurnSection; | ||
| summary = summaryWithPreservedTurns; | ||
| if (quality.ok || attempt >= totalAttempts - 1) { | ||
| break; |
There was a problem hiding this comment.
When qualityGuardEnabled = true but qualityGuardMaxRetries = 0 (reachable via resolveQualityGuardMaxRetries(-1) → 0), totalAttempts = 1. The quality audit runs but the condition attempt >= totalAttempts - 1 evaluates to true on the first iteration (0 >= 0), causing an immediate break without any retry attempt. The quality feedback is generated but discarded, making the retry feature silently inactive.
This is a valid configuration path (the clamping test explicitly checks resolveQualityGuardMaxRetries(-1) === 0), but callers enabling the guard with 0 retries will see no retry behavior without any warning, making the feature a no-op.
Consider either:
- Logging a warning when the guard is enabled with 0 retries, or
- Documenting that
qualityGuardMaxRetries: 0disables the retry loop while keeping the audit flag semantically enabled
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/pi-extensions/compaction-safeguard.ts
Line: 917-932
Comment:
When `qualityGuardEnabled = true` but `qualityGuardMaxRetries = 0` (reachable via `resolveQualityGuardMaxRetries(-1) → 0`), `totalAttempts = 1`. The quality audit runs but the condition `attempt >= totalAttempts - 1` evaluates to true on the first iteration (`0 >= 0`), causing an immediate break without any retry attempt. The quality feedback is generated but discarded, making the retry feature silently inactive.
This is a valid configuration path (the clamping test explicitly checks `resolveQualityGuardMaxRetries(-1) === 0`), but callers enabling the guard with 0 retries will see no retry behavior without any warning, making the feature a no-op.
Consider either:
1. Logging a warning when the guard is enabled with 0 retries, or
2. Documenting that `qualityGuardMaxRetries: 0` disables the retry loop while keeping the audit flag semantically enabled
How can I resolve this? If you propose a fix, please make it concise.|
@jalehman This is my next follow up, Thank you! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c8fe6b7b9
ℹ️ 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".
| summaryWithoutPreservedTurns = historySummary.trim() | ||
| ? `${historySummary}\n\n---\n\n${splitTurnSection}` | ||
| : splitTurnSection; |
There was a problem hiding this comment.
Audit history summary before appending split-turn section
When isSplitTurn is true, summaryWithoutPreservedTurns is built by concatenating historySummary with splitTurnSection, and the quality audit runs on that combined text. If the main historySummary omits required headings, but the split-turn prefix summary still contains them, auditSummaryQuality can incorrectly pass and skip retries. This makes the new quality guard ineffective for split-turn compactions, because headings from the prefix can mask structural regressions in the actual history summary.
Useful? React with 👍 / 👎.
1c8fe6b to
4f37e30
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f37e30dc0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| summaryWithoutPreservedTurns = historySummary.trim() | ||
| ? `${historySummary}\n\n---\n\n${splitTurnSection}` | ||
| : splitTurnSection; |
There was a problem hiding this comment.
Audit history summary before appending split-turn section
In split-turn compactions, summaryWithoutPreservedTurns is built by concatenating historySummary with splitTurnSection before auditSummaryQuality runs, so required-heading checks can be satisfied by headings that exist only in the prefix summary. This means a malformed historySummary (missing ## Decisions, etc.) can still pass quality audit whenever the split-turn prefix contains those headings, causing retries to be skipped and degraded history summaries to persist in split-turn sessions.
Useful? React with 👍 / 👎.
4f37e30 to
be473ef
Compare
Summary
Describe the problem and fix in 2–5 bullets:
wrapUntrustedPromptDataBlockinsrc/agents/sanitize-for-prompt.tsand consuming it from compaction safeguard (no local sanitizer duplication).qualityGuardEnableddefaults tofalseunless explicitly configured).Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
User-visible / Behavior Changes
qualityGuardEnabled.Security Impact (required)
No)No)No)No)No)Yes, explain risk + mitigation:Repro + Verification
Environment
Steps
pnpm test src/agents/pi-extensions/compaction-safeguard.test.ts src/agents/sanitize-for-prompt.test.tspnpm tsgopnpm formatExpected
Actual
Evidence
Attach at least one:
Human Verification (required)
What you personally verified (not just CI), and how:
Compatibility / Migration
Yes)No)No)Failure Recovery (if this breaks)
qualityGuardEnabledunset/false, or revert this PR.src/agents/pi-extensions/compaction-safeguard.ts,src/agents/sanitize-for-prompt.tsRisks and Mitigations
List only real risks for this PR. Add/remove entries as needed. If none, write
None.Stack: 4/9, depends on #25555.