diff --git a/devlog/2026-09-20_quality-rejection-no-leak/REQ.md b/devlog/2026-09-20_quality-rejection-no-leak/REQ.md new file mode 100644 index 00000000..d7acea7d --- /dev/null +++ b/devlog/2026-09-20_quality-rejection-no-leak/REQ.md @@ -0,0 +1,100 @@ +# REQ - Quality-gate rejection: stop leaking internal details into model context + +- Task ID: `2026-09-20_quality-rejection-no-leak` +- Home Repo: `opencode-acp` +- Created: 2026-09-20 +- Status: InProgress +- Priority: P2 +- Owner: ranxianglei +- References: https://github.com/ranxianglei/opencode-acp/issues/444 + +## 1. Background & Problem Statement + +- **Context**: When the pre-commit quality gate rejects a compression, + `buildQualityRejectionError` (`lib/compress/quality-gate/rejection.ts`) throws + an error whose message is returned to the model as the tool result. The #397 + fix already removed the giant embedded `HOW_TO_COMPRESS_RULES` block, but the + remaining message still leaks internal implementation detail that the model + does not need in order to act. +- **Current behavior (symptom)** — three residual leaks (issue #444): + 1. `"Full compression rules are already in your system prompt."` — reveals an + internal architecture fact (that a system prompt carries the full rules). + 2. `"...add \"acknowledgeRisk\": true to bypass the quality gate on your next + compress call."` — frames `acknowledgeRisk` as an internal *bypass/backdoor* + mechanism rather than a neutral, documented tool parameter. + 3. Algorithm-internal diagnostic lines — `Gate layer:`, `rougeF1:`, + `top20Recall:` — plus threshold config carried inside `result.reason` by the + external algorithm package (e.g. `(threshold: 200 chars OR 0.5% retention)`). +- **Impact**: every rejection injects these tokens into the live context of a + session that is (by definition) already large. It is pure overhead with no + actionable value for the model, and it exposes internals that should stay in + the ACP debug log. +- **Expected behavior** (per issue #444): keep the blocking gate; on failure the + model should learn (a) why it failed and (b) how to retry, concisely. No system + prompt / internal-rules references, no "bypass" framing, no algorithm metrics or + threshold config. Detailed diagnostics belong in the ACP log, not the model + context. + +## 2. Reproduction (if applicable) + +- **Environment**: any session with `qualityGate.enabled: true`; model calls + `compress` with a summary that fails L1/L2 (issue report: range m00006–m00010, + ~36234 tokens → 670-char summary, L1-length, retention 0.46%). +- **Minimal reproduction steps**: + 1) Enable the quality gate, call `compress` with a too-short summary. + 2) Read the tool error: it contains the system-prompt sentence, the "bypass" + framing, and the Gate layer / rougeF1 / top20Recall lines. + +## 3. Constraints & Non-Goals + +- **Constraints**: + - E2E fake-LLM detection markers `"COMPRESSION REJECTED"` and + `"QUALITY GATE FAILURE"` MUST remain in the message header + (`scripts/e2e/fake-llm-server.ts`). + - The message MUST still contain: the range, the core failure reason + ("Summary too short" / "Content coverage too low"), original token count, + summary char count, a `Retry:` directive, and the `acknowledgeRisk` escape + hatch (neutral wording). Existing unit assertions depend on these substrings. + - Message length regression guard `< 1000` chars must hold (and improve). + - No change to the gate evaluation itself, its config schema, or the + throw/bypass mechanics in `lib/compress/range.ts`. +- **Non-Goals** (out of scope): + - Advisory mode / auto-bypass after N rejections (#339, owner decision pending). + - Removing `dangerous` parameter residue (#339). + - Changing the external `context-compress-algorithms` package's own `reason` + string format — we sanitize at display time instead. + +## 4. Acceptance Criteria (must be testable) + +- **Correctness**: + - [ ] Rejection message includes: header (with both detection markers), range, + core reason, original token count, summary char count, retention, `Retry:` + directive, and neutral `acknowledgeRisk` mention. + - [ ] Rejection message does NOT include: `"system prompt"`, `"bypass"`, + `"Gate layer"`, `"rougeF1"`, `"top20Recall"`, or any `(threshold: …)` text. + - [ ] Core reason strips the algorithm's trailing threshold/config detail + (`"Summary too short: …(threshold: …)"` → `"Summary too short"`). + - [ ] Full diagnostics (full reason incl. thresholds, gate layer, rougeF1, + top20Recall, ratio) are written to the ACP logger when one is supplied. + - [ ] Message length stays < 1000 chars (and is shorter than before the fix). +- **Regression**: + - [ ] Existing assertions in `tests/quality-gate-enforcement.test.ts` + (header/range/reason/tokens/chars/Retry/acknowledgeRisk, absence of HOW TO + COMPRESS / KEEP VERBATIM / CRITICAL, length guard) still pass. + - [ ] Integration tests (rejection through real tool, acknowledgeRisk bypass, + #301 preemptive no-op) unchanged and green. Full suite green. + +## 5. Proposed Approach + +- **Affected modules & entry files**: + - `lib/compress/quality-gate/rejection.ts` — add `coreReason()` sanitizer; drop + Gate layer / rougeF1 / top20Recall lines from the model-facing message; + rewrite retry paragraph (remove system-prompt sentence + "bypass" framing); + add optional logger param that emits full diagnostics via `warn`. + - `lib/compress/range.ts` — pass `ctx.logger` into `buildQualityRejectionError`. + - `tests/quality-gate-enforcement.test.ts` — add no-leak + debug-log-path + assertions. +- **Risks**: very low — message content + one call-site arg; single throw site; + E2E markers preserved; logger param is optional so other callers are unaffected. +- **Rollback strategy**: revert the commit; purely textual/log change, no state or + API impact. diff --git a/devlog/2026-09-20_quality-rejection-no-leak/WORKLOG.md b/devlog/2026-09-20_quality-rejection-no-leak/WORKLOG.md new file mode 100644 index 00000000..a3e4f261 --- /dev/null +++ b/devlog/2026-09-20_quality-rejection-no-leak/WORKLOG.md @@ -0,0 +1,75 @@ +# WORKLOG - Quality-gate rejection: stop leaking internal details into model context + +- Task ID: `2026-09-20_quality-rejection-no-leak` +- Home Repo: `opencode-acp` +- Branch: `2026-09-20_quality-rejection-no-leak` +- References: https://github.com/ranxianglei/opencode-acp/issues/444 + +## 1. Commits + +| # | Hash | Summary | +|---|-------|---------| +| 1 | TBD | fix: keep quality-gate rejection concise — strip internal details, log full diagnostics (#444) | + +## 2. Key Files & Changes + +### `lib/compress/quality-gate/rejection.ts` +- Added `RejectionDiagnosticsLogger` structural interface (`warn(message, data?)`) so the + module can emit diagnostics without a runtime dependency on the concrete `Logger` class + and so tests can inject a plain mock without casting. +- Added `coreReason(reason)` sanitizer: cuts an algorithm-provided reason at the first + `:` or `(`, stripping internal threshold/config detail that the external + `context-compress-algorithms` package appends (e.g. `(threshold: 200 chars OR 0.5% retention)`). +- Rewrote the model-facing message. Removed: + - `"Full compression rules are already in your system prompt."` (architecture leak) + - `"…to bypass the quality gate on your next compress call."` (internal-bypass framing) + - `Gate layer:` / `rougeF1:` / `top20Recall:` lines (algorithm-internal metrics) + - the redundant `Ratio:` line and the "the gate re-evaluates automatically" clause +- Now emits the FULL diagnostic record (full reason incl. thresholds, layer, originalTokens, + summaryChars, ratio, retentionPct, rougeF1, top20Recall) via `logger?.warn(...)` when a logger + is supplied — i.e. detailed info goes to the ACP log, not the model context. +- `buildQualityRejectionError(plan, result, logger?)` — new optional third param; existing 2-arg + callers are unaffected. +- Message length dropped from ~780 chars to ~430 chars for a representative L1 rejection. + +### `lib/compress/range.ts` +- Passes `ctx.logger` as the third argument to `buildQualityRejectionError(...)` (the single + throw site) so full diagnostics land in the ACP log. + +### `tests/quality-gate-enforcement.test.ts` +- New fixture `buildLeakFixture()` mirroring the real algorithm-package reason shape. +- New tests: + - `rejection keeps actionable info but drops internal details (#444)` — asserts presence of + header markers / range / core reason / Retry / acknowledgeRisk AND absence of + "system prompt", "bypass", "Gate layer", "rougeF1", "top20Recall", "(threshold:". + - `coreReason strips colon- and parenthetical-carried threshold detail (#444)` — table-driven + across L1 colon form, L2 parenthetical form, L2 colon form, and bare reason. + - `full rejection diagnostics are written to the logger, not the message (#444)` — asserts one + warn record carries full reason/layer/rougeF1/top20Recall while none appear in the message. + - `buildQualityRejectionError works without a logger (optional param)` — no-throw guard. + +## 3. Test Results + +- `npm run typecheck`: PASS (clean). +- Target file `tests/quality-gate-enforcement.test.ts`: 19/19 PASS (was 15 before this change). +- Full suite `node --import tsx --test tests/*.test.ts`: **1294 pass / 2 fail**. + - Both failures are PRE-EXISTING and ENVIRONMENTAL, unrelated to this change: + - `tests/soft-block.test.ts` → `EACCES: permission denied, mkdir '/tmp/opencode-dcp-dangerous-*'` + - `tests/inactive-block-decompress.test.ts` → same read-only `/tmp` cause + - Proven pre-existing by stashing all three changed files and re-running on clean HEAD: the + same 2 tests fail identically there. This sandbox mounts `/tmp` read-only; those tests create + temp dirs under `/tmp`. Not introduced or fixed here (out of scope). +- Mutation verification (§5.7.3): temporarily restored the OLD buggy `rejection.ts`; the 3 leak + assertions FAILED against it (no-leak, coreReason strip, diagnostics-to-logger), then restored + the fix → all green. Confirms the regression tests genuinely catch the bug. + +## 4. Lessons Learned + +- The #397 fix removed the giant embedded rule block but left finer-grained leaks (architecture + sentence, "bypass" framing, algorithm metrics, threshold config). "Concise enough" was not the + bar — "no internals in model context" is. Distinguish *actionable* content from *diagnostic* + content: the former stays in the tool result, the latter belongs in the ACP log. +- Keep E2E detection markers (`COMPRESSION REJECTED`, `QUALITY GATE FAILURE`) stable in the + header — `scripts/e2e/fake-llm-server.ts` greps for them. +- Sanitize at display time rather than depending on the external algorithm package's `reason` + string staying terse. diff --git a/lib/compress/quality-gate/rejection.ts b/lib/compress/quality-gate/rejection.ts index e6d669ca..df1ee36f 100644 --- a/lib/compress/quality-gate/rejection.ts +++ b/lib/compress/quality-gate/rejection.ts @@ -8,6 +8,15 @@ export interface RejectionPlanInfo { messageTokenById: Map } +/** + * Minimal logging surface for full rejection diagnostics. Kept structural (not + * the concrete `Logger` class) so tests can inject a plain mock without casting, + * and so this module has no runtime dependency on the logger implementation. + */ +export interface RejectionDiagnosticsLogger { + warn(message: string, data?: Record): void +} + function formatMetric(result: QualityGateResult, name: string): string { const m = result.metrics.find((x) => x.name === name) if (!m) return "?" @@ -38,32 +47,51 @@ function computeStats(plan: RejectionPlanInfo): { return { originalTokens, summaryChars, ratio, retentionPct } } +/** + * Reduce an algorithm-provided reason to its core phrase. The external + * `context-compress-algorithms` package appends internal threshold/config detail + * after a colon or inside a parenthetical (e.g. `"Summary too short: 670 chars … + * (threshold: 200 chars OR 0.5% retention)"`). Only the phrase before the first + * `:` / `(` is actionable for the model, so everything after it stays out of the + * model-facing message (#444). + */ +function coreReason(reason: string | undefined): string { + if (!reason) return "unknown" + const candidates = [reason.indexOf(":"), reason.indexOf("(")].filter((i) => i >= 0) + const cut = Math.min(...candidates) + return (cut > 0 ? reason.slice(0, cut) : reason).trim() || "unknown" +} + export function buildQualityRejectionError( plan: RejectionPlanInfo, result: QualityGateResult, + logger?: RejectionDiagnosticsLogger, ): Error { const stats = computeStats(plan) - const metrics = [ - `Reason: ${result.reason || "unknown"}`, - `Original: ~${stats.originalTokens} tokens`, - `Summary: ${stats.summaryChars} chars`, - `Ratio: ${stats.ratio}:1`, - `Retention: ${stats.retentionPct}%`, - `Gate layer: ${result.layer ?? "unknown"}`, - `rougeF1: ${formatMetric(result, "rougeF1")}`, - `top20Recall: ${formatMetric(result, "top20Recall")}`, - ] - // Keep this payload small: it is returned to the model as tool output on - // every rejection, and the full HOW TO COMPRESS rules already live in the - // system prompt (lib/prompts/system.ts) — re-injecting them here defeats - // compression. Restate only a targeted retry hint. + // Full diagnostics go to the ACP log, NOT the model context. The model only + // needs enough to act (what failed + how to retry); algorithm-internal + // metrics and threshold config are diagnostic detail that would otherwise + // consume context on every rejection (#444). + logger?.warn("quality gate rejected compression", { + range: `${plan.startId}–${plan.endId}`, + reason: result.reason || "unknown", + layer: result.layer ?? "unknown", + originalTokens: stats.originalTokens, + summaryChars: stats.summaryChars, + ratio: stats.ratio, + retentionPct: stats.retentionPct, + rougeF1: formatMetric(result, "rougeF1"), + top20Recall: formatMetric(result, "top20Recall"), + }) + const message = `⚠️ COMPRESSION REJECTED — QUALITY GATE FAILURE Range: ${plan.startId}–${plan.endId} -${metrics.join("\n")} +Reason: ${coreReason(result.reason)} +Original: ~${stats.originalTokens} tokens · Summary: ${stats.summaryChars} chars · Retention: ${stats.retentionPct}% -Retry: rewrite a more complete summary that preserves critical details (file paths, decisions, exact values, errors) and call compress again on the same range — the gate re-evaluates automatically. Full compression rules are already in your system prompt. If you are confident the summary is correct despite the metrics, add "acknowledgeRisk": true to bypass the quality gate on your next compress call.` +Retry: write a more complete summary preserving critical details (file paths, decisions, exact values, errors), then call compress again on the same range. If you are confident the summary is adequate despite the metrics, pass "acknowledgeRisk": true.` return new Error(message) } diff --git a/lib/compress/range.ts b/lib/compress/range.ts index 38709373..47a4f7d3 100644 --- a/lib/compress/range.ts +++ b/lib/compress/range.ts @@ -302,6 +302,7 @@ export function createCompressRangeTool(factoryCtx: ToolFactoryContext): ReturnT messageTokenById: plan.selection.messageTokenById, }, result, + ctx.logger, ) } } diff --git a/tests/quality-gate-enforcement.test.ts b/tests/quality-gate-enforcement.test.ts index 38f39736..b7516dec 100644 --- a/tests/quality-gate-enforcement.test.ts +++ b/tests/quality-gate-enforcement.test.ts @@ -283,6 +283,102 @@ test("buildQualityRejectionError computes ratio and retention from plan data", ( assert.ok(error.message.includes("100 chars"), "should show summary chars") }) +// === Issue #444: rejection must not leak internal details into model context === + +function buildLeakFixture() { + const messageIds = ["msg-1"] + const plan = { + startId: "m00006", + endId: "m00010", + summary: "x".repeat(670), + messageIds, + messageTokenById: buildTokenMap(messageIds, 36234), + } + const result: QualityGateResult = { + passed: false, + layer: "L1-length", + // Mirrors the real algorithm-package reason shape (core + values + threshold config). + reason: + "Summary too short: 670 chars, 0.46% retention (threshold: 200 chars OR 0.5% retention)", + metrics: [ + { name: "rougeF1", value: 0.01, format: "ratio" }, + { name: "top20Recall", value: 0.05, format: "ratio" }, + ], + } + return { plan, result } +} + +test("rejection keeps actionable info but drops internal details (#444)", () => { + const { plan, result } = buildLeakFixture() + const msg = buildQualityRejectionError(plan, result).message + + assert.ok(msg.includes("COMPRESSION REJECTED"), "E2E detection marker present") + assert.ok(msg.includes("QUALITY GATE FAILURE"), "E2E detection marker present") + assert.ok(msg.includes("m00006–m00010"), "range preserved") + assert.ok(msg.includes("Summary too short"), "core failure reason preserved") + assert.ok(msg.includes("Retry:"), "retry directive preserved") + assert.ok(msg.includes("acknowledgeRisk"), "escape hatch discoverable") + + // Internal implementation detail must NOT reach the model context (#444). + assert.ok(!msg.includes("system prompt"), "no system-prompt architecture reference") + assert.ok(!msg.includes("bypass"), "no internal-bypass framing") + assert.ok(!msg.includes("Gate layer"), "no gate-layer metric") + assert.ok(!msg.includes("rougeF1"), "no rougeF1 metric") + assert.ok(!msg.includes("top20Recall"), "no top20Recall metric") + assert.ok(!msg.includes("(threshold:"), "no threshold config leak") +}) + +test("coreReason strips colon- and parenthetical-carried threshold detail (#444)", () => { + const base = { + startId: "m00001", + endId: "m00002", + summary: "s", + messageIds: ["m1"], + messageTokenById: buildTokenMap(["m1"], 1000), + } + const cases: Array<[string, string]> = [ + [ + "Summary too short: 670 chars, 0.46% retention (threshold: 200 chars OR 0.5% retention)", + "Summary too short", + ], + ["Content coverage too low (threshold: F1 < 0.05 AND recall < 0.20)", "Content coverage too low"], + ["Content coverage too low: rougeF1 below floor", "Content coverage too low"], + ["Summary too short", "Summary too short"], + ] + for (const [reason, expected] of cases) { + const msg = buildQualityRejectionError(base, { passed: false, reason, metrics: [] }).message + assert.ok(msg.includes(`Reason: ${expected}`), `expected "${expected}" for reason "${reason}"`) + assert.ok(!msg.includes("(threshold"), `must not leak threshold for "${reason}"`) + } +}) + +test("full rejection diagnostics are written to the logger, not the message (#444)", () => { + const { plan, result } = buildLeakFixture() + const calls: Array<{ message: string; data?: Record }> = [] + const mockLogger = { warn: (message: string, data?: Record) => calls.push({ message, data }) } + + const msg = buildQualityRejectionError(plan, result, mockLogger).message + + // Exactly one diagnostic record emitted with the full detail. + assert.equal(calls.length, 1, "one diagnostic log entry") + const data = calls[0].data ?? {} + assert.equal(data.reason, result.reason, "full reason (incl. thresholds) logged") + assert.equal(data.layer, "L1-length", "gate layer logged") + assert.equal(String(data.rougeF1), "0.0100", "rougeF1 logged") + assert.equal(String(data.top20Recall), "0.0500", "top20Recall logged") + + // ...and none of it appears in the model-facing message. + assert.ok(!msg.includes("0.0100"), "rougeF1 value absent from message") + assert.ok(!msg.includes("L1-length"), "gate layer absent from message") +}) + +test("buildQualityRejectionError works without a logger (optional param)", () => { + const { plan, result } = buildLeakFixture() + // Must not throw when no logger is supplied. + const msg = buildQualityRejectionError(plan, result).message + assert.ok(msg.includes("COMPRESSION REJECTED"), "header present without logger") +}) + test("qualityGateRetryPending defaults to false", () => { const state = createSessionState() assert.equal(state.qualityGateRetryPending, false)