fix(learning): stop treating guidance retrieval as successful reuse - #698
Conversation
proffesor-for-testing
left a comment
There was a problem hiding this comment.
The implementation and focused regression suite look sound (23/23 passed locally), but this PR is not merge-ready because the required PR-body failure-mode contract is missing and the Lint PR body required check is red. Please add a ## Failure modes section plus the issue #401 checked attestation, mapping each stated risk to a test or tracking issue, then let CI rerun. Once that required gate is green, the code review has no additional blocker.
proffesor-for-testing
left a comment
There was a problem hiding this comment.
The earlier blocker (missing Failure modes section) is resolved and the Lint PR body check is green. Verified the change: guidance and routing retrieval no longer write fabricated success rows to experience_applications; application counts and savings are only recorded via an explicit outcome call, and the adapter/ExperienceReplay counters now agree. The regression suite runs against the real adapter, native HNSW, and the in-memory unified SQLite. Build passes; focused suite 44/44, consumer suites 75/75. Codex adversarial pass confirmed the one known gap (no production caller records outcomes yet), which the PR body discloses and which is strictly safer than the previous fabricated writes.
350e627
into
proffesor-for-testing:main
|
Thank you, @rudycelekli! Merely retrieving past guidance was being recorded as a successful reuse even when the task then failed, so confidence scores and token-savings stats were inflated with evidence that never happened. This removes those premature writes and adds a real end-to-end regression suite proving retrieval no longer manufactures success. Merged. One follow-up worth tracking: after this change nothing in production calls |
Summary
Requesting experience guidance currently records every source as a successful application before the task executes. These rows survive task failure and feed subsequent quality reinforcement. In a built MCP reproduction, two
task_orchestratecalls failed withmissing sourceFiles, but both persisted successful experience applications. In a separate real-adapter consolidation experiment, four guidance reads followed by one failed reuse produced an 80% recorded success rate and confidence 0.58, versus 0.26 for the failure-only control.Remove the premature writes from both
getExperienceGuidanceandrouteTaskWithExperience. Preserve returned guidance and the explicit success/failure API. Count applications and savings after explicit persistence succeeds; repeated retrieval no longer adds projected savings to application totals.CI follow-up
Includes the fork-comment guard from #701 and the runner/reporting correction from #704. Optional fork comments are skipped while artifacts remain available; nonzero runner exits and timeouts remain failures. Coverage uses valid reporters and produces a JSON summary, while JUnit remains a test report.
The 12 fork-comment regressions and nine runner-exit regressions pass. A real 18-test coverage control produces the expected reports with exit 0; an invalid-reporter control retains exit 1 despite all 18 tests passing. Earlier green coverage jobs did not establish completed coverage reporting: the old wrapper could mask the invalid coverage reporter's error. The current full run has now completed successfully with the corrected wrapper.
These reporting and exit-status failure modes are exercised by
tests/unit/scripts/fork-pr-comments.test.ts,tests/unit/scripts/ci-vitest-run.test.ts, and the real reporter controls described in #704.Verification
Final published head
fb525399: all 18 checks pass. Full coverage job: 23,610 tests passed / 62 skipped; 976 files passed / six skipped. Coverage artifacts upload successfully; measured line coverage is 65.46% (the existing 80% comparison is advisory). No reporter errors or exit normalization occur.Nine regressions use production replay, native HNSW, SQLite, trajectories and consolidation; only embedding HTTP is a deterministic local fixture. Unpatched: 7 fail / 2 pass. Patched: 9 pass.
Related replay/consolidation regression run: 23 tests pass across 4 files.
Built MCP before/after: both tasks still receive guidance and fail the same validation; successful application rows change from 2 to 0. SQLite integrity is
okin both fresh fixtures.npm run buildandnpx eslint src/integrations/agentic-flow/reasoning-bank/index.tspass.Focused regression command (23 tests across 4 files):
No schema or historical data changes. No in-repository production caller currently invokes
recordExperienceApplication; correlating completed task outcomes with retrieved source IDs remains separate work. This change prevents fabricated success evidence and preserves the explicit API. It does not claim to complete automatic outcome attribution or bypass/fix provenance-based promotion gates.Failure modes
The committed regression file,
tests/integration/experience-guidance-outcomes.test.ts, exercises the following cases:returns guidance repeatedly without recording applications or successful reuseandincludes routing guidance without recording a completed experience.does not turn failed execution after retrieval into positive reinforcement, including real consolidation and its resulting confidence.persists explicit success and failure with matching adapter and replay counts, plus the two parameterizedreinforces explicitly recorded success=$success through consolidationcases.keeps predicted savings from subsequent guidance separate from recorded savings.leaves application evidence and counters unchanged when no guidance exists.does not count an outcome whose SQLite persistence fails, using a real foreign-key rejection.Required check (issue #401)