Skip to content

feat(improvement): remeasure selected candidate changes before promotion - #149

Merged
drewstone merged 7 commits into
mainfrom
feat/measured-partial-promotion
Aug 17, 2026
Merged

feat(improvement): remeasure selected candidate changes before promotion#149
drewstone merged 7 commits into
mainfrom
feat/measured-partial-promotion

Conversation

@drewstone

Copy link
Copy Markdown
Contributor

The unsafe shortcut this avoids

A whole knowledge candidate can pass while an arbitrary subset of its file changes fails: a selected page may lose its supporting source, break a wikilink, regress readiness, or become a duplicate. Filtering the already-approved activation plan would silently inherit the whole candidate's evidence for a hybrid that was never measured.

Change

Add improveSelectedKnowledgeCandidate, which derives a caller-selected subset from one previously measured candidate and then routes that subset through the ordinary knowledge-improvement evidence and promotion path as a new candidate.

The helper:

  • resolves the source candidate's frozen baseline and measured snapshot;
  • verifies the source transition plan still matches promotionPlanHash;
  • admits only exact changed source-candidate paths, rejecting unknown, repeated, and generated knowledge/index.md paths;
  • binds the source candidate, source evidence/plan, selected paths, caller policy identity, rationale, and JSON-safe policy output into a deterministic selection digest;
  • materializes the subset into an isolated baseline under the canonical recoverable mutation lock;
  • recomputes the generated index;
  • proves the derived candidate changed exactly the selected non-derived paths;
  • runs the existing validation, readiness, KB-quality, RAG, answer-quality, and caller evaluators;
  • persists the selection identity inside lifecycle evidence, so the ordinary candidate evidence hash binds the subset;
  • emits an idempotent selection.json receipt beside the derived candidate;
  • returns an ordinary candidate reference consumed by the existing promoteKnowledgeCandidate path.

There is no second activation path and no evidence reuse from the broad source candidate.

Tests

The suite proves:

  • the exact subset is present during evaluation, the dropped file is absent, and ordinary promotion applies only the remeasured subset;
  • a harmful claim-only subset fails its own evaluator even though the whole claim+support candidate passed;
  • rejected derived candidates cannot be promoted;
  • unknown, repeated, and generated paths are refused;
  • reopening the same selected run after deleting its mutable workspace returns the same frozen measured candidate and receipt without rerunning the update.

This closes the partial-promotion gap from Discovery's adoption audit with a stronger invariant than filtering an activation plan: every promoted subset has its own evidence.

tangletools
tangletools previously approved these changes Aug 17, 2026

@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 — 2aaf8485

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.

This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.

tangletools · auto-approval · reason: drewstone_author · 2026-08-17T02:46:28Z

tangletools
tangletools previously approved these changes Aug 17, 2026

@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 — 4d11513c

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.

This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.

tangletools · auto-approval · reason: drewstone_author · 2026-08-17T03:13:22Z

@tangletools

Copy link
Copy Markdown
Contributor

⚠️ Review Interrupted — 4d11513c

The review runner stopped before publishing a final verdict: webhook_restarted.

State Detail
Interrupted webhook restarted

No review verdict was produced for this run. Trigger a fresh review on the current PR head if the PR is still open.

tangletools · #149 · model: kimi-for-coding · updated 2026-08-17T03:24:35Z

tangletools
tangletools previously approved these changes Aug 17, 2026

@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 — 4d11513c

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.

This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.

tangletools · auto-approval · reason: drewstone_author · 2026-08-17T03:27:31Z

@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-with-nits

Verdict sound-with-nits
Coverage 2 of 2 lenses (value, usefulness)
Concerns 2 (2 weak-concern)
Heuristic 0.0s
Duplication 0.0s
Interrogation 370.9s (2 bridge agents)
Total 370.9s

💰 Value — sound-with-nits

Adds improveSelectedKnowledgeCandidate, which remeasures a caller-selected subset of an already-measured candidate through the ordinary improve→promote path so no unmeasured hybrid can ever be promoted; sound and in-grain, with two small duplicate-helper nits.

  • What it does: Adds a new exported helper (src/kb-improvement/selected-candidate.ts:143) that takes a previously measured KnowledgeImprovementCandidateRef plus a list of changed file paths, and produces a NEW measured candidate containing only those changes. It resolves the frozen baseline + candidate snapshot via withKnowledgeImprovementComparison (workspace.ts:50), verifies the source plan hash still equals th
  • Goals it achieves: Eliminates the correctness hole where a whole candidate passes its evaluators but an arbitrary subset of its changes is promoted anyway (a page losing its supporting source, breaking a wikilink, regressing readiness, or becoming a duplicate). Once merged, every promoted change is a measured candidate: a caller cannot filter an approved plan and silently inherit the whole candidate's evidence. It a
  • Assessment: Good. It is coherent and sits squarely in the codebase's grain: it reuses improveKnowledgeBase + promoteKnowledgeCandidate rather than adding a parallel activation path, and follows the exact composition pattern already established by optimizeKnowledgeBasePolicy (src/kb-improvement/optimization.ts:88, which also wraps improveKnowledgeBase with a custom updateKnowledge callback). The integrity disc
  • Better / existing approach: No materially better architecture exists. I searched src/ (grep for subset/partial/selectedPaths/selection and read transition.ts, workspace.ts, optimization.ts, activation.ts, evaluation.ts, contracts.ts) and found no existing primitive that re-measures a path-filtered subset of a measured candidate; optimizeKnowledgeBasePolicy materializes a policy, not a file subset, and 'selectionScenarios' el
  • Model: opencode/deepseek/deepseek-v4-pro
  • Bridge attempts: 3
  • Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error; opencode/zai-coding-plan/glm-5.2: opencode: opencode error

🎯 Usefulness — sound

A safe partial-promotion capability that remeasures a caller-selected subset as an ordinary candidate, built entirely from the package's existing measurement and activation machinery with no second promotion path.

  • Integration: Exported at the package root (src/kb-improvement.ts:43-49) and composes only established primitives: improveKnowledgeBase with a custom updateKnowledge callback (the same extension point optimizeKnowledgeBasePolicy already uses at src/kb-improvement/optimization.ts:143), withKnowledgeImprovementComparison (src/kb-improvement/workspace.ts:50), the canonical file-transaction/mutation-lock stack, and
  • Fit with existing patterns: Fits the codebase's central invariant rather than competing with it: promotion is gated on promotionPlanHash/evidenceHash binding exactly the measured content (assertCandidateEvidence, src/kb-improvement/workspace.ts:209-243; assertCandidateTransitionPlan, src/kb-improvement/transition.ts:557-569). The obvious cheaper design — filtering the activation plan — would violate that invariant, and grepp
  • Real-world viability: Holds up beyond the happy path: crash recovery routes through the same resumeTransaction validation the promotion path uses (src/kb-improvement/selected-candidate.ts:251-258), re-invocation is idempotent without re-running evaluators (test at tests/kb-improvement/selected-candidate.test.ts:190-226), source-content drift is caught by re-hashing each file before materialization (selected-candidate.t
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

💰 Value Audit

🟡 immutableJson re-implements the already-exported immutableJsonValue [duplication] ``

selected-candidate.ts:476 defines a module-private deep-freeze helper identical in effect to immutableJsonValue (activation.ts:294), which is exported and already used by workspace.ts:17. Import and reuse the existing helper instead of a third local deep-freeze copy.

🟡 selectionMutations duplicates knowledgePlanMutations [duplication] ``

selected-candidate.ts:410 (selectionMutations: read file, verify sha256+mode, emit KnowledgeFileMutation) is nearly identical to knowledgePlanMutations in transition.ts:540. They differ only in the error message ('changed before subset materialization' vs 'changed before activation'). Could be unified into one shared 'mutations for a filtered plan' helper, but this is a ~15-line overlap and does not gate shipping.


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 · 20260817T033408Z

@tangletools

Copy link
Copy Markdown
Contributor

❌ Needs Work — 4d11513c

Review health 100/100 · Reviewer score 26/100 · Confidence 75/100 · 18 findings (5 high, 1 medium, 12 low)

opencode GLM 5.2 opencode DeepSeek v4 Pro opencode DeepSeek v4 Flash aggregate
Readiness 27 61 26 26
Confidence 75 75 75 75
Correctness 27 61 26 26
Security 27 61 26 26
Testing 27 61 26 26
Architecture 27 61 26 26

Reviewer score is advisory once the run is complete and the verdict has no blockers.

Full multi-shot audit completed 3/3 planned shots over 3 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 3/3 planned shots over 3 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 3/3 planned shots over 3 changed files. Global verifier still owns final merge decision.

Blocking

🔴 HIGH Failing assertion: error-message regex does not match the thrown error — tests/kb-improvement/selected-candidate.test.ts

expect(() => knowledgeImprovementCandidateRef(selected)).toThrow(/not ready for promotion/) fails. knowledgeImprovementCandidateRef -> candidateRefFor throws knowledge candidate 'kcand_...' is not ready (src/kb-improvement/workspace.ts:116), which does not contain the substring 'for promotion'. The wording '...not ready for ${action}' exists only in src/kb-improvement/transition.ts:244, a path this call never reaches. Reproduced: pnpm vitest run tests/kb-improvement/selected-candidate.test.ts -> 'Tests 1 failed | 3 passed (4)'. Fix: assert /is not ready/ (or /not ready/), or align the production message with the intended regex. As written, this test is red and blocks CI.

🔴 HIGH Failing assertion: toThrow regex never matches actual error message — tests/kb-improvement/selected-candidate.test.ts

expect(() => knowledgeImprovementCandidateRef(selected)).toThrow(/not ready for promotion/) — actual throw is knowledge candidate 'kcand_8550c04dd61c0d62' is not ready from candidateRefFor (src/kb-improvement/workspace.ts:116, pre-existing, unchanged by this PR). The 'not ready for promotion' wording only exists in src/kb-improvement/transition.ts:244 (knowledge candidate is not ready for ${action}) on the promote/restore path, which this direct call never hits. Verified by executing the suite: assertion fails deterministically across 3 runs (earlier toMatchObject({status:'rejected'}) and toMatchObject({passed:false}) pass, so only the regex is wrong). CI runs pnpm test (vitest run) and will be red. Fix: change to .toThrow(/is not ready/) or assert the exact message.

🔴 HIGH Head commit ships a failing test and a lint error (CI red) — tests/kb-improvement/selected-candidate.test.ts

Verified by running pnpm vitest run tests/kb-improvement/selected-candidate.test.ts --pool=threads --maxWorkers=1: Test Files 1 failed, Tests 1 failed | 3 passed. The failing assertion expect(() => knowledgeImprovementCandidateRef(selected)).toThrow(/not ready for promotion/) never matches the actual thrown message knowledge candidate 'kcand_...' is not ready (thrown by candidateRefFor in src/kb-improvement/workspace.ts:116). Product behavior is correct (a rejected candidate refuses promotion), but the test's expected-message regex is wrong, so the PR's test suite is red on head. Additionally pnpm lint (biome check src tests) fails on this same test file (formatter would add a trailing newline after [line 227](https://github.com/tangle-network/agent-knowledge/blob/4d11513c922ac4a68

🔴 HIGH New test fails deterministically at head: wrong error-message regex — tests/kb-improvement/selected-candidate.test.ts

Test asserts expect(() => knowledgeImprovementCandidateRef(selected)).toThrow(/not ready for promotion/), but the exported helper resolves to candidateRefFor in src/kb-improvement/workspace.ts:116 which throws knowledge candidate '<id>' is not ready for a rejected candidate. /not ready for promotion/.test("knowledge candidate 'kcand_a255de868da4f9c7' is not ready") is false — verified by a complete vitest run of this file (3 passed, 1 failed, exact assertion diff shown) and by static regex evaluation. Impact: CI is red at PR head; the production behavior itself is correct (rejected candidates correctly refuse a ref). Fix: change the regex to /is not ready/ or equivalent. One-line test fix, but merge-blocking until applied. (Cross-shot evidence: the test file belongs to another shot; repo

🔴 HIGH Test 2 error-message assertion can never pass — tests/kb-improvement/selected-candidate.test.ts

expect(() => knowledgeImprovementCandidateRef(selected)).toThrow(/not ready for promotion/) asserts an error that the implementation does not produce. knowledgeImprovementCandidateRef (workspace.ts:42) calls candidateRefFor, which for a rejected candidate throws knowledge candidate '<candidateId>' is not ready (workspace.ts:116). RegExp /not ready for promotion/.test('knowledge candidate kimpsel-... is not ready') is false, so vitest's toThrow (chai throws -> message match) fails every run. The test always fails regardless of implementation behavior. Fix: match /not ready$/ or /is not ready/, or assert on selected.candidate.status === 'rejected' instead of the throw message.

Other

🟠 MEDIUM Base-drift precondition is unvalidated; subset derivation fails late with misleading errors — src/kb-improvement/selected-candidate.ts

The helper requires options.root to be byte-identical to the source candidate's frozen baseline, but never verifies hashKnowledgeBase(options.root) === sourceCandidate.baseHash up front. The proof is in the comparison references: assertExactSelectedChanges (line 265) compares the derived candidate against source.baseline.root, assertSelectionTransaction (line 233) requires the transaction's beforeHash (read from the live-root copy, file-transaction.ts prepareKnowledgeFileTransaction) to equal the SOURCE

🟡 LOW Receipt type exported without its runtime zod schema — src/kb-improvement.ts

The barrel exports MeasuredKnowledgeSelectionReceipt as an inferred type, but the corresponding measuredSelectionReceiptSchema (selected-candidate.ts:66) is not exported as a value — unlike the established contracts pattern where persisted-record schemas are re-exported (KnowledgeImprovementRunStateSchema, KnowledgeImprovementEvidenceSchema, KnowledgeImprovementCandidateRefSchema at lines 32-34). The receipt is persisted to candidates//selection.json and re-parsed internally on resume (selected-candidate.ts:463), so downstream consumers reading that file from disk cannot re-validate it with the canonical schema and must hand-roll a structurally identical one.

🟡 LOW Receipt zod schemas not exported from the public barrel — src/kb-improvement.ts

The new module defines measuredSelectionReceiptSchema/measuredSelectionLifecycleSchema (selected-candidate.ts:53-85), and the receipt type is public (MeasuredKnowledgeSelectionReceipt). But kb-improvement.ts only exports the types and the function, not the schemas. contracts.ts sets the package precedent of exporting its schemas as values (kb-improvement.ts:31-35), and package.json's exports map publishes only the index, so a consumer who reads candidates//selection.json cannot validate it without re-implementing the schema. Fix: add export { measuredSelectionLifecycleSchema, measuredSelectionReceiptSchema } from './kb-improvement/selected-candidate' to the barrel, consistent with the sibling module.

🟡 LOW DERIVED_KNOWLEDGE_PATHS only lists knowledge/index.md, inconsistent with isScaffoldPath — src/kb-improvement/selected-candidate.ts

DERIVED_KNOWLEDGE_PATHS = new Set(['knowledge/index.md']) is used by notDerivedPath to exclude 'derived' paths from the selectable changed set and from assertExactSelectedChanges. store.ts:isScaffoldPath (lines 56-63) treats knowledge/index.md, knowledge/log.md, AND any nested

/index.md or /log.md as non-searchable scaffold pages. So knowledge/log.md and nested scaffold paths are NOT excluded here: if a source candidate's update/research hook modified such a path, it would be exposed as an ordinary selectable file and carried into the derived subset instead of being treated as derived content. Impact is low (content is still remeasure

🟡 LOW No up-front check that the live root still equals the source candidate's baseline — src/kb-improvement/selected-candidate.ts

Selection is defined against the frozen source baseline, but the derived run copies the LIVE root as its baseline (run.ts:68-69) and nothing compares the two up front. When the live KB has drifted (e.g. another candidate was promoted since), failure arrives late and indirect: drift on a selected path makes prepareKnowledgeFileTransaction's beforeHash (from the live workspace copy) disagree with the plan hash frozen from the source baseline, yielding 'selected knowledge transaction does not match its approved path set' (line 432); drift on unselected paths surfaces as 'selected knowledge candidate changed the wrong files' ([line 446](https://github

🟡 LOW Re-running the helper after the derived candidate is promoted throws an opaque ZodError — src/kb-improvement/selected-candidate.ts

After promoteKnowledgeCandidate promotes the derived candidate, a second improveSelectedKnowledgeCandidate call with identical inputs makes improveKnowledgeBase take its promoted early-return path (src/kb-improvement/run.ts:99-123), which returns no lifecycle and never invokes updateKnowledge, so in-process lifecycleSelection is also undefined. measuredSelectionLifecycleSchema.parse(result.lifecycle?.knowledgeUpdate?.metadata?.selection ?? lifecycleSelection) then parses undefined and throws a raw ZodError before the clearer receipt-conflict check in persistSelectionReceipt (line 466-467) can produce 'measured knowledge selection receipt

🟡 LOW Selection receipt does not bind the base the derived candidate was actually measured against — src/kb-improvement/selected-candidate.ts

receiptWithoutHash records sourceCandidate, sourcePlanHash, selectedCandidateHash, selectedPlanHash, selectedEvidenceHash, and derivedCandidateId, but not the derived candidate's baseHash. The derived run snapshots its baseline from the live root at derived-run start (run.ts createBaselineSnapshot), which under the current code only ever equals the source baseHash when the base has not drifted (see the medium finding above). A verifier reading the receipt later cannot confirm that the measured subset was measured against the base named by sourceCandidate, because the derived evidence.baseHash lives only in the candidate's evidence.json. Adding a derivedBaseHash (or asserting equality with sourceCandidate.baseHash) to the receipt would make the source-base/derived-base relationship audita

🟡 LOW Type-omitted update-driver fields are not stripped at runtime — src/kb-improvement/selected-candidate.ts

improvementOptions destructures out the selection-specific fields but not acquireKnowledge, step, or knowledgeResearch, which are excluded only by the Omit<> on ImproveSelectedKnowledgeCandidateOptions. A JavaScript or as any caller passing acquireKnowledge would smuggle a second knowledge-update driver into the derived run (evaluation.ts:73-81 allows one driver; acquireKnowledge would run before the selection hook inside the same update loop, letting unselected content enter the candidate). updateKnowledge, enabledPhases, requiredPhases, root, goal, runId, and implementationRef are safe because explicit keys after the spread ([lines 208-216](

const result = await improveKnowledgeBase({

🟡 LOW recordedSelection fallback throws a confusing ZodError when the derived run is already promoted — src/kb-improvement/selected-candidate.ts

recordedSelection = measuredSelectionLifecycleSchema.parse(result.lifecycle?.knowledgeUpdate?.metadata?.selection ?? lifecycleSelection). If a caller reuses an options.runId whose run is already 'promoted', improveKnowledgeBase (run.ts:99-123) returns early with no lifecycle, and updateKnowledge never runs, so both operands are undefined and zod throws a bare 'Invalid input' ZodError with no run context. The subsequent binding check (lines 297-304) never executes. Not a data-integrity bug (fail-closed), but the diagnostic hides the real cause. Fix: guard first, e.g. throw new Error('selected knowledge candidate produced no measured selection

🟡 LOW Missing trailing newline at EOF fails formatter/lint — tests/kb-improvement/selected-candidate.test.ts

The file ends on }) with no trailing newline. pnpm biome check tests/kb-improvement/selected-candidate.test.ts reports 'Formatter would have printed the following content' and exits non-zero. The repo lint script is biome check src tests, so this file fails pnpm lint. Fix: add the trailing newline (biome format --write).

🟡 LOW Missing trailing newline fails pnpm lint (biome check) in CI — tests/kb-improvement/selected-candidate.test.ts

File has no newline at EOF (diff shows '\ No newline at end of file'). npx biome check tests/kb-improvement/selected-candidate.test.ts emits 1 format error ('Found 1 error') proposing to add the trailing blank line. CI runs pnpm lint = biome check src tests (ci.yml:26), so this file alone reds the lint gate. Fix: biome format --write or append newline.

🟡 LOW Missing trailing newline fails repo lint gate — tests/kb-improvement/selected-candidate.test.ts

File ends at '})' with no final newline. biome check tests/kb-improvement/selected-candidate.test.ts reports a formatter error (verified locally: 'Formatter would have printed...'), so pnpm lint (biome check src tests) fails on this file. Add the terminating newline.

🟡 LOW Test title claims validation happens 'before opening a derived run' but never verifies it — tests/kb-improvement/selected-candidate.test.ts

The three rejects.toThrow assertions verify the error messages, but nothing asserts that no derived run directory was created under the KB root (e.g., checking the runs dir is empty after the throws). The ordering claim in the test name — the safety property that malformed selections cost nothing — is unverified. Add an assertion that no new run dir exists for the selection digest after each rejection.


tangletools · 2026-08-17T04:13:49Z · trace

@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.

❌ 5 Blocking Findings — 4d11513c

Full multi-shot audit completed 3/3 planned shots over 3 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 3/3 planned shots over 3 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 3/3 planned shots over 3 changed files. Global verifier still owns final merge decision.

Full immutable report for this review: trace

Summary comment for this run: full summary


tangletools · 2026-08-17T04:13:49Z · immutable trace

…ally throws

candidateRefFor throws "knowledge candidate '<id>' is not ready". The
assertion expected "not ready for promotion", a wording that exists only
on the promote path this call never reaches, so the test failed on every
run. Match the thrown message and terminate the file with a newline for
biome.
@drewstone

Copy link
Copy Markdown
Contributor Author

@tangletools review now

tangletools
tangletools previously approved these changes Aug 17, 2026

@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 — 3d5dd9b5

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.

This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.

tangletools · auto-approval · reason: drewstone_author · 2026-08-17T05:23:06Z

@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 did not run — no verdict

This is not an approval and not a clean bill of health. Neither interrogation lens returned a judgment, so this PR has no value-audit evidence for or against it.

Status audit-incomplete (could not run)
Why value: cli-bridge admission rejected (queue saturated); usefulness: cli-bridge admission rejected (queue saturated)
Lenses answered 0 of 2
What to do re-run once the CLI bridge has capacity: pr-reviewerctl trigger <repo>#<pr> --force

💰 Value — error

value agent never ran: the CLI bridge refused admission (no model was started).

  • Model: opencode/deepseek/deepseek-v4-pro
  • Bridge attempts: 4
  • Bridge error: opencode/kimi-for-coding/k2p7: Bridge returned 503: bridge at capacity (queue_timeout, lane=reserved): active=20/20 queued=0/48 — no model was started

🎯 Usefulness — error

usefulness agent never ran: the CLI bridge refused admission (no model was started).

  • Model: opencode/deepseek/deepseek-v4-pro
  • Bridge attempts: 4
  • Bridge error: opencode/zai-coding-plan/glm-5.2: Bridge returned 503: bridge at capacity (queue_timeout, lane=reserved): active=20/20 queued=1/48 — no model was started

No concerns are listed because nothing examined the change — absence of findings here is absence of evidence, not a pass.


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 · 20260817T055151Z

@tangletools
tangletools dismissed stale reviews from themself August 17, 2026 05:51

Value audit could not run (value: cli-bridge admission rejected (queue saturated); usefulness: cli-bridge admission rejected (queue saturated)). This approval was provisional on that audit running, so it is dismissed. Re-run the reviewer once the CLI bridge has capacity.

@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 did not run — no verdict

This is not an approval and not a clean bill of health. Neither interrogation lens returned a judgment, so this PR has no value-audit evidence for or against it.

Status audit-incomplete (could not run)
Why value: cli-bridge admission rejected (queue saturated); usefulness: cli-bridge admission rejected (queue saturated)
Lenses answered 0 of 2
What to do re-run once the CLI bridge has capacity: pr-reviewerctl trigger <repo>#<pr> --force

💰 Value — error

value agent never ran: the CLI bridge refused admission (no model was started).

  • Model: opencode/deepseek/deepseek-v4-pro
  • Bridge attempts: 5
  • Bridge error: opencode/kimi-for-coding/k2p7: opencode: opencode error; opencode/zai-coding-plan/glm-5.2: Bridge returned 503: bridge at capacity (queue_timeout, lane=reserved): active=20/20 queued=6/48 — no model was started

🎯 Usefulness — error

usefulness agent never ran: the CLI bridge refused admission (no model was started).

  • Model: opencode/deepseek/deepseek-v4-pro
  • Bridge attempts: 4
  • Bridge error: opencode/zai-coding-plan/glm-5.2: Bridge returned 503: bridge at capacity (queue_timeout, lane=reserved): active=20/20 queued=12/48 — no model was started

No concerns are listed because nothing examined the change — absence of findings here is absence of evidence, not a pass.


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 · 20260817T060302Z

@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 did not run — no verdict

This is not an approval and not a clean bill of health. Neither interrogation lens returned a judgment, so this PR has no value-audit evidence for or against it.

Status audit-incomplete (could not run)
Why value: cli-bridge admission rejected (queue saturated); usefulness: cli-bridge admission rejected (queue saturated)
Lenses answered 0 of 2
What to do re-run once the CLI bridge has capacity: pr-reviewerctl trigger <repo>#<pr> --force

💰 Value — error

value agent never ran: the CLI bridge refused admission (no model was started).

  • Model: opencode/deepseek/deepseek-v4-pro
  • Bridge attempts: 4
  • Bridge error: opencode/kimi-for-coding/k2p7: Bridge returned 503: bridge at capacity (queue_timeout, lane=reserved): active=20/20 queued=19/48 — no model was started

🎯 Usefulness — error

usefulness agent never ran: the CLI bridge refused admission (no model was started).

  • Model: opencode/deepseek/deepseek-v4-pro
  • Bridge attempts: 4
  • Bridge error: opencode/zai-coding-plan/glm-5.2: Bridge returned 503: bridge at capacity (queue_timeout, lane=reserved): active=20/20 queued=18/48 — no model was started

No concerns are listed because nothing examined the change — absence of findings here is absence of evidence, not a pass.


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 · 20260817T061025Z

@tangletools

Copy link
Copy Markdown
Contributor

✅ No Blockers — 3d5dd9b5

Review health 100/100 · Reviewer score 86/100 · Confidence 75/100 · 3 findings (3 low)

deepseek: Correctness 86 · Security 86 · Testing 86 · Architecture 86

Reviewer score is advisory once the run is complete and the verdict has no blockers.

Full multi-shot audit completed 3/3 planned shots over 3 changed files. Global verifier still owns final merge decision.

🟡 LOW Untested selection edge paths — src/kb-improvement/selected-candidate.ts

tests/kb-improvement/selected-candidate.test.ts covers happy-path subset, harmful-subset rejection, malformed path rejection, and resume/idempotency. It does not exercise: empty selectedPaths (the 'null candidate' branch where selectionMutationPlanHash = contentHash([]) and applied=false), a deletion entry (afterHash === null), a mode-only change (planEntryChanged second clause), or recovery of a crash that leaves the file transaction pending (lock.recovery branch, lines 230-246). These branches carry non-trivial binding logic (assertSelectionTransaction, assertExactSelectedChanges) and are currently verified only by inspection. Add tests for

🟡 LOW knowledge/index.md is misclassified as a derived path — src/kb-improvement/selected-candidate.ts

DERIVED_KNOWLEDGE_PATHS = {'knowledge/index.md'} is treated as a file the indexer regenerates, but it is not. writeKnowledgeIndex -> FileSystemKbStore.putIndex writes KB_INDEX_PATH = '.agent-knowledge/index.json' (kb-store.ts:40,296-301), never knowledge/index.md. knowledge/index.md and knowledge/log.md are scaffold pages written exactly once by initKnowledgeBase via writeIfMissing (store.ts:76-77) and are excluded from the page index via isScaffoldPath (store.ts:47-63). Consequences: (1) a source candidate that legitimately changes knowledge/index.md has that change silently dropped from changedSourcePlan (line 170) and masked in assertExactSelec

🟡 LOW Bare catch can mask non-ENOENT read failures in the dependency-check evaluator — tests/kb-improvement/selected-candidate.test.ts

The evaluator in the 'harmful subset' test does try { await readFile(...support.md) ... } catch { return { score: 0, passed: false, ... } }. Any readFile error — permission denial, an unexpected candidateRoot path, a thrown error from the harness — is folded into the same 'missing page' failing metric. The test's later assertions (rejected status, passed: false, ENOENT on root/claim.md) still pass, so an unrelated failure mode would produce a green test with a misleading 'claim is missing its supporting page' note. Tighten by asserting the caught error is ENOENT (e.g. catch (e) { expect(e).toMatchObject({ code: 'ENOENT' }); return ... }) so only true file-absence drives the rejection. Test-only impact; no runtime behavior affected.


tangletools · 2026-08-17T06:27:01Z · trace

@tangletools
tangletools dismissed their stale review August 17, 2026 06:27

Superseded by re-review — no blocking findings on latest commit.

@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.

✅ Approved — 3 non-blocking findings — 3d5dd9b5

Full multi-shot audit completed 3/3 planned shots over 3 changed files. Global verifier still owns final merge decision.

Full immutable report for this review: trace

Summary comment for this run: full summary


tangletools · 2026-08-17T06:27:01Z · immutable trace

@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
Coverage 1 of 2 lenses (usefulness)
Concerns 1 (1 weak-concern)
Heuristic 0.0s
Duplication 0.0s
Interrogation 665.0s (2 bridge agents)
Total 665.0s

⚠️ Partial audit — the verdict covers only usefulness. value: cli-bridge admission rejected (queue saturated). Treat the missing lens as unexamined, not as clear.

💰 Value — error

value agent never ran: the CLI bridge refused admission (no model was started).

  • Model: opencode/deepseek/deepseek-v4-pro
  • Bridge attempts: 4
  • Bridge error: opencode/kimi-for-coding/k2p7: Bridge returned 503: bridge at capacity (queue_timeout, lane=reserved): active=20/20 queued=15/48 — no model was started

🎯 Usefulness — sound

A well-fitted library capability that fills a real gap — sub-selecting a measured candidate's changes has no safe route today because promotion's plan-hash binding structurally rejects any filtered plan — and it closes that gap by routing the subset through the same single measurement-and-promotion

  • Integration: Exported at the package root (src/kb-improvement.ts:43-49 -> src/index.ts:27 'export * from ./kb-improvement') of the published library @tangle-network/agent-knowledge@8.0.7, so it is reachable by every downstream consumer on publish. In-repo callers are tests only (tests/kb-improvement/selected-candidate.test.ts), which is the standing pattern for this surface: promoteKnowledgeCandidate (transiti
  • Fit with existing patterns: Follows the codebase's established composition pattern exactly: optimizeKnowledgeBasePolicy (optimization.ts:143-177) already wraps improveKnowledgeBase with a derived deterministic implementationRef, a stableId runId, and a custom updateKnowledge that materializes a derived candidate and binds metadata into lifecycle evidence — this helper is the same grain applied to path selection. It reuses th
  • Real-world viability: Built past the happy path: crash recovery via resumeTransaction with plan-hash validation (selected-candidate.ts:251-258); tamper detection at three points — source plan re-verified against the frozen promotionPlanHash (167-169), each file re-hashed before materialization (417-421), and post-materialization exact-change assertion (261-265); evidence-to-candidate binding re-checked after the run (2
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 4

🎯 Usefulness Audit

🟡 Empty-selection null-candidate path is deliberately admitted but unproven [robustness] ``

selected-candidate.ts:280-284 explicitly supports selectedPaths: [] ('measuring the exact baseline as a null candidate'), yielding a candidate-ready run whose promotion would be a content no-op; the suite (tests/kb-improvement/selected-candidate.test.ts) never exercises it, so its evaluator/receipt/promotion behavior is asserted only by construction. Either add one test pinning the null-candidate outcome or reject empty selections at the boundary — worth a reviewer's eye, but it flows the ordina


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 · 20260817T063004Z

@drewstone

Copy link
Copy Markdown
Contributor Author

Disposition of the 3 non-blocking LOW findings on 3d5dd9b5, plus the local proof I ran myself in an isolated worktree at that exact head.

Local verification (worktree ak-partial-promo, HEAD 3d5dd9b)

  • pnpm lint — biome checked 216 files, no findings.
  • pnpm typechecktsc --noEmit on src and on tsconfig.contracts.json, both clean.
  • pnpm test — 68 files passed, 3 skipped; 646 tests passed, 12 skipped; 0 failed.
  • CI on this head: ci pass, official-optimizers pass.

Finding 1 — untested selection edge paths (LOW). Accepted as a coverage gap, not a defect. The named branches (empty selectedPaths, deletion entry, mode-only change, lock recovery) are reachable and are verified by inspection only. Tracked for a follow-up test pass; it does not change behavior on this head.

Finding 2 — knowledge/index.md misclassified as a derived path (LOW). Partly correct, and the consequence does not hold. Correct: knowledge/index.md is never regenerated by the indexer. FileSystemKbStore.putIndex writes KB_INDEX_PATH = .agent-knowledge/index.json (kb-store.ts:40,296), so the constant name DERIVED_KNOWLEDGE_PATHS describes the wrong mechanism. Incorrect: the change is not silently dropped in an unsafe direction. notDerivedPath is applied symmetrically — once when building changedSourcePlan (selected-candidate.ts:170) and again inside assertExactSelectedChanges (selected-candidate.ts:442). The path is excluded from the selectable set and from the set the assertion compares against, so no selected candidate can pass with an unaccounted change. knowledge/index.md is a scaffold page written once by initKnowledgeBase through writeIfMissing (store.ts:76) and excluded from the page index by isScaffoldPath (store.ts:115). Holding it out of promotion is the conservative and intended behavior. The residual defect is the constant's name, which should say scaffold rather than derived. Cosmetic, no behavior change, deferred rather than pushed here because a push dismisses this approval.

Finding 3 — bare catch in the dependency-check evaluator (LOW). Accepted, test-only. The catch in the harmful-subset test folds any readFile error into the missing-page metric. Asserting code: 'ENOENT' would tighten it. No runtime path is affected.

No finding blocks merge. Merging on the multi-shot No Blockers verdict for this head.

@drewstone
drewstone merged commit a31bca5 into main Aug 17, 2026
2 checks 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