Skip to content

perf(prompt): stabilize review cache prefix - #163

Closed
SergeBrick wants to merge 1 commit into
openclaw:mainfrom
SergeBrick:fix/review-prompt-cache-prefix
Closed

perf(prompt): stabilize review cache prefix#163
SergeBrick wants to merge 1 commit into
openclaw:mainfrom
SergeBrick:fix/review-prompt-cache-prefix

Conversation

@SergeBrick

Copy link
Copy Markdown

Summary

  • place shared review policy and the JSON contract before feature-specific metadata
  • keep CUDA guidance, evidence paths, prompt context, and file excerpts in the dynamic suffix
  • add a regression test proving different features retain the same shared prefix

Why

Clawpatch starts a provider call per feature. Provider prompt caches require an exact matching prefix, but the feature record previously appeared before most shared review instructions. That made the common instruction tail ineligible for reuse across feature reviews.

This change only reorders existing prompt content; the provider output schema and validation behavior are unchanged.

Checks

  • corepack pnpm typecheck
  • corepack pnpm lint
  • corepack pnpm format:check
  • corepack pnpm test (890 passed, 1 skipped)
  • corepack pnpm build

@SergeBrick
SergeBrick requested a review from a team as a code owner July 24, 2026 04:38
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. labels Jul 24, 2026
@clawsweeper

clawsweeper Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed August 1, 2026, 9:42 PM ET / August 2, 2026, 01:42 UTC.

ClawSweeper review

What this changes

Reorders review prompts so shared policy and the JSON response contract precede per-feature project, evidence, and file context, then adds a test that two features share that prefix.

Merge readiness

Blocked until real behavior proof is added - 5 items remain

Keep this PR open: the prompt reordering is focused and has a useful regression test, but the current branch has a concrete CHANGELOG.md merge conflict and lacks real after-fix provider evidence that cache reuse improves across feature reviews.

Priority: P3
Reviewed head: d4536efe52ab9636eefe9f6b98a262359dc298c4

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) The patch is focused and regression-tested, but a merge conflict and missing real provider-run proof leave it below merge-ready quality.
Proof confidence 🧂 unranked krab (1/6) Needs real behavior proof before merge: The PR body lists typecheck, lint, test, format, and build checks, but it provides no redacted after-fix provider transcript, telemetry, log, or terminal artifact demonstrating cache reuse across distinct feature reviews. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The PR body lists typecheck, lint, test, format, and build checks, but it provides no redacted after-fix provider transcript, telemetry, log, or terminal artifact demonstrating cache reuse across distinct feature reviews. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 5 items Current main still uses the old prompt ordering: Current main puts project and feature metadata before the shared review policy, evidence-path list, prompt context, and JSON schema; the proposed cache-prefix ordering is not already implemented.
Focused regression coverage: The branch adds a prompt-builder test that compares the prefix before Feature: for two distinct features and asserts that the shared policy and JSON contract are in that prefix.
Concrete merge conflict: A three-way merge against current main conflicts in CHANGELOG.md: main added the Rust seed-context entry while this branch adds the cache-reuse entry at the same unreleased-section location.
Findings 1 actionable finding [P2] Resolve the changelog conflict before merging
Security None None.

How this fits together

Clawpatch maps a repository into semantic features, then builds one provider-review prompt per feature. The provider returns structured findings that are schema-validated and fed into the review workflow, so prompt ordering affects both cache reuse and model-facing review context.

flowchart LR
  Project[Mapped project] --> Prompt[Review prompt builder]
  Feature[Semantic feature] --> Prompt
  Files[Selected source and test excerpts] --> Prompt
  Prompt --> Provider[Review provider]
  Provider --> Validation[Schema validation]
  Validation --> Findings[Review findings]
Loading

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The PR body lists typecheck, lint, test, format, and build checks, but it provides no redacted after-fix provider transcript, telemetry, log, or terminal artifact demonstrating cache reuse across distinct feature reviews. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Resolve the changelog conflict before merging (P2) - CHANGELOG.md has a concrete three-way conflict with current main's unreleased entry, so this branch is currently dirty and cannot be merged until it is rebased and the two entries are reconciled.
  • Resolve merge risk (P1) - The branch cannot merge cleanly until its CHANGELOG.md edit is reconciled with the current unreleased section.
  • Resolve merge risk (P1) - Prompt cache reuse and unchanged provider-review behavior remain unproven in a real provider run; unit coverage only verifies generated prompt text.
  • Complete next step (P2) - The remaining blockers are a branch rebase and contributor-supplied real behavior proof, neither of which is a safe autonomous repair task.

Findings

  • [P2] Resolve the changelog conflict before merging — CHANGELOG.md:5
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Changed surface 3 files; 53 added, 15 removed The implementation is narrowly scoped to prompt construction, regression coverage, and one release-note entry.
Merge conflicts 1 file conflicts CHANGELOG.md conflicts with the current unreleased-section entry on main and blocks a clean merge.

Merge-risk options

Maintainer options:

  1. Rebase and demonstrate provider behavior (recommended)
    Resolve the current changelog conflict and add redacted terminal output, logs, or provider telemetry showing cache reuse across two distinct feature reviews with valid structured output.
  2. Pause pending operational proof
    Keep the PR open but do not merge if the contributor cannot provide real provider-run evidence for the cache-performance claim.

Technical review

Best possible solution:

Rebase onto current main, resolve the changelog conflict without disturbing the prompt boundary, and attach redacted after-fix provider telemetry or terminal output showing two feature reviews reuse the shared prefix/cache while returning valid review output.

Do we have a high-confidence way to reproduce the issue?

No high-confidence real provider reproduction is included. The new unit test proves prompt-text ordering, but it does not show cache reuse or after-fix structured output from a provider.

Is this the best way to solve the issue?

Yes, the proposed static-prefix ordering and focused regression test are a maintainable approach, provided the branch is rebased and real provider behavior confirms the performance claim.

Full review comments:

  • [P2] Resolve the changelog conflict before merging — CHANGELOG.md:5
    CHANGELOG.md has a concrete three-way conflict with current main's unreleased entry, so this branch is currently dirty and cannot be merged until it is rebased and the two entries are reconciled.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.98

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 4a5028c27f01.

Labels

Label justifications:

  • P3: This is a bounded review-prompt cache optimization with no evidence of an urgent user-facing regression.
  • merge-risk: 🚨 other: Reordering instructions and context can change provider-review behavior, and no real provider run yet proves the intended cache outcome preserves valid output.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body lists typecheck, lint, test, format, and build checks, but it provides no redacted after-fix provider transcript, telemetry, log, or terminal artifact demonstrating cache reuse across distinct feature reviews. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

  • Current main still uses the old prompt ordering: Current main puts project and feature metadata before the shared review policy, evidence-path list, prompt context, and JSON schema; the proposed cache-prefix ordering is not already implemented. (src/prompt.ts:201, 4a5028c27f01)
  • Focused regression coverage: The branch adds a prompt-builder test that compares the prefix before Feature: for two distinct features and asserts that the shared policy and JSON contract are in that prefix. (src/prompt.test.ts:12, d4536efe52ab)
  • Concrete merge conflict: A three-way merge against current main conflicts in CHANGELOG.md: main added the Rust seed-context entry while this branch adds the cache-reuse entry at the same unreleased-section location. (CHANGELOG.md:5, 4a5028c27f01)
  • Feature-history provenance: Available blame attributes the current prompt-builder implementation and adjacent prompt tests to the v0.7.1 release commit, making Peter Steinberger the strongest current-history routing candidate for this surface. (src/prompt.ts:197, f6e00010cc2a)
  • Release and main-only check: No local release tag contains the branch commit, and current main still has the pre-reordering source, so this work is neither shipped nor superseded by main. (src/prompt.ts:201, d4536efe52ab)

Likely related people:

  • Peter Steinberger: The available blame and history attribute the current prompt construction and adjacent prompt tests to the v0.7.1 release commit. (role: current prompt-surface contributor; confidence: medium; commits: f6e00010cc2a; files: src/prompt.ts, src/prompt.test.ts)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Rebase and resolve the CHANGELOG.md conflict against current main.
  • Add redacted terminal output, logs, or provider telemetry proving cache reuse and valid review output for two distinct features.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (24 earlier review cycles; latest 8 shown)
  • reviewed 2026-07-31T23:34:32.063Z sha d4536ef :: needs real behavior proof before merge. :: [P2] Remove the release-owned changelog entry
  • reviewed 2026-08-01T07:10:37.189Z sha d4536ef :: needs real behavior proof before merge. :: none
  • reviewed 2026-08-01T09:35:13.853Z sha d4536ef :: needs real behavior proof before merge. :: none
  • reviewed 2026-08-01T10:56:01.448Z sha d4536ef :: needs real behavior proof before merge. :: none
  • reviewed 2026-08-01T12:44:02.231Z sha d4536ef :: needs real behavior proof before merge. :: none
  • reviewed 2026-08-01T14:27:38.943Z sha d4536ef :: needs real behavior proof before merge. :: [P3] Remove the release-owned changelog entry
  • reviewed 2026-08-01T20:13:57.314Z sha d4536ef :: needs real behavior proof before merge. :: [P3] Remove the release-owned changelog entry
  • reviewed 2026-08-01T22:36:34.197Z sha d4536ef :: needs real behavior proof before merge. :: [P3] Remove the release-owned changelog entry

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Jul 29, 2026
@steipete

steipete commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

I reproduced this on the built CLI with two different features reviewed sequentially through the real Codex provider (--jobs 1). Both reviews completed, but the second provider session reported cached_input_tokens: 0, so I could not live-prove cross-feature cache reuse.

The structural test passes, but it only proves that the prompt text before Feature: is identical. On the built branch that common prefix is 3,032 bytes, approximately 758 tokens. That is not sufficient evidence that a provider will cache or reuse it, and the real sequential run did not do so.

Focused proof otherwise passed: 454 tests across prompt, mapper, and Rust mapper coverage; formatting; type-checking; linting; and production build. The problem is specifically that the performance claim did not survive live validation. My triage recommendation is CLOSE rather than land an unverified prompt reordering.

@steipete steipete closed this Aug 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants