Skip to content

fix(improvement): keep promoted run reopening read-only#70

Merged
drewstone merged 1 commit into
mainfrom
fix/proposal-only-resume
Jul 18, 2026
Merged

fix(improvement): keep promoted run reopening read-only#70
drewstone merged 1 commit into
mainfrom
fix/proposal-only-resume

Conversation

@drewstone

Copy link
Copy Markdown
Contributor

Problem

Reopening a promoted improveKnowledgeBase() run entered the candidate transition path. It could rewrite derived knowledge metadata and conflated candidate generation with explicit activation.

Solution

Return the already measured candidate without invoking promotion, while holding the knowledge lock and rejecting live-hash drift or pending activation recovery. Bump the package to 3.2.1.

Verification

  • pnpm lint
  • pnpm typecheck
  • pnpm test (291 passed, 10 skipped live-network cases)
  • pnpm build
  • pnpm verify:package
  • independent adversarial re-review: prior medium finding resolved

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-approved drewstone PR — 345829de

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

tangletools · auto-approval · reason: drewstone_author · 2026-07-18T02:21:03Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Value Audit — sound

Verdict sound
Concerns 0 (none)
Heuristic 0.0s
Duplication 0.0s
Interrogation 102.2s (2 bridge agents)
Total 102.2s

💰 Value — sound

Reopening an already-promoted improvement run now returns the measured candidate read-only (hash-checked) instead of re-entering the promotion transition path — clean separation of candidate generation from activation, built on existing primitives.

  • What it does: Before this change, calling improveKnowledgeBase() on a run whose state.status === 'promoted' routed back into applyKnowledgeCandidateTarget(..., 'candidate') (src/kb-improvement.ts:991) — the full promotion-transition machinery: transaction planning, recovery handling, activation persistence, derived-metadata writes. After the change (src/kb-improvement.ts:844-867), the promoted branch takes the
  • Goals it achieves: Separate candidate generation (improveKnowledgeBase) from explicit activation (promoteKnowledgeCandidate at src/kb-improvement.ts:725). A reopened promoted run is a terminal state; re-entering promotion could rewrite derived knowledge metadata, re-run transaction planning, and conflate 'I am asking for a candidate' with 'I am reactivating.' The fix makes reopening idempotent and read-only, and fai
  • Assessment: Good change, in the grain of the codebase. It mirrors the existing terminal-state early-return pattern already used for state.status === 'blocked' (src/kb-improvement.ts:871). It reuses the four exact primitives the file already trusts: withKnowledgeMutation, hashKnowledgeBase, assertCandidateEvidence, and candidateRefFor. The hash check matches the contract already enforced inside applyKnowledgeC
  • Better / existing approach: none — this is the right approach. I checked for an existing 'read-only reopen of a terminal run' helper (grep for withKnowledgeRead / loadKnowledgeImprovementStateFromRun / assertCandidateEvidence usages in src/) and there is no pre-existing helper that returns a promoted candidate without invoking transitions; assertCandidateEvidence is the closest primitive and the fix correctly composes it inl
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 2
  • Bridge warning: opencode/kimi-for-coding/k2p7: bridge stream ended without value-audit content

🎯 Usefulness — sound

A minimal correctness fix that keeps reopening a promoted improvement run read-only, cleanly separating candidate measurement from activation and matching established in-file patterns.

  • Integration: Reachable via the public improveKnowledgeBase export whenever persisted run state on disk is 'promoted' — the normal resume/reopen path for any long-lived agent workflow. The promoted branch returns the canonical KnowledgeImprovementResult shape (src/kb-improvement.ts:157-165) and reuses assertCandidateEvidence, the same evidence-validation helper used by the non-promoted success path (src/kb-impr
  • Fit with existing patterns: Strong fit. Before the fix, improveKnowledgeBase leaked into the activation path via applyKnowledgeCandidateTarget('candidate'), duplicating what promoteKnowledgeCandidate (src/kb-improvement.ts:725-729) already owns. The fix enforces the exact separation both functions' docstrings describe: improveKnowledgeBase measures/grows a candidate; promoteKnowledgeCandidate activates it. No competing patte
  • Real-world viability: Holds up. Concurrency is covered by withKnowledgeMutation (mutation-lock.ts:87), which acquires the durable file lock. Live-hash drift is rejected by the explicit currentHash !== promoted.candidateHash check. Pending activation recovery is rejected because withKnowledgeMutation with no resumeTransaction throws on any pending transaction (mutation-lock.ts:144-148). Stored evidence tampering is caug
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

No concerns — sound change, no better or existing approach found. ✅


What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass What it asks
Heuristic Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication Do added function/class names already exist elsewhere in the repo?
Value Audit What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

value-audit · 20260718T022432Z

@drewstone
drewstone merged commit b3589bb into main Jul 18, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants