Skip to content

fix(learning): stop treating guidance retrieval as successful reuse - #698

Merged
proffesor-for-testing merged 4 commits into
proffesor-for-testing:mainfrom
rudycelekli:fix/experience-guidance-outcomes
Sep 22, 2026
Merged

proffesor-for-testing merged 4 commits into
proffesor-for-testing:mainfrom
rudycelekli:fix/experience-guidance-outcomes

Conversation

@rudycelekli

@rudycelekli rudycelekli commented Sep 20, 2026 •

Copy link
Copy Markdown
Contributor

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_orchestrate calls failed with missing 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 getExperienceGuidance and routeTaskWithExperience. 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 ok in both fresh fixtures.

  • npm run build and npx eslint src/integrations/agentic-flow/reasoning-bank/index.ts pass.

Focused regression command (23 tests across 4 files):

AQE_MEMORY_BACKEND=memory npx vitest run \
  tests/integration/experience-guidance-outcomes.test.ts \
  tests/unit/learning/experience-replay-initialization.test.ts \
  tests/unit/learning/experience-consolidation-contradiction.test.ts \
  tests/unit/learning/experience-consolidation-safety-valve.test.ts --bail=0

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:

  • Retrieval or routing records successful reuse before execution: returns guidance repeatedly without recording applications or successful reuse and includes routing guidance without recording a completed experience.
  • A failed execution gains positive reinforcement from an earlier guidance read: does not turn failed execution after retrieval into positive reinforcement, including real consolidation and its resulting confidence.
  • Explicit outcomes or their counters diverge: persists explicit success and failure with matching adapter and replay counts, plus the two parameterized reinforces explicitly recorded success=$success through consolidation cases.
  • Subsequent guidance adds predicted savings to recorded savings: keeps predicted savings from subsequent guidance separate from recorded savings.
  • A lookup with no guidance creates application evidence: leaves application evidence and counters unchanged when no guidance exists.
  • A failed SQLite write increments application counters: does not count an outcome whose SQLite persistence fails, using a real foreign-key rejection.

Required check (issue #401)

  • Every failure mode mentioned in this PR description has either (a) a test that exercises it, or (b) a linked tracking issue.

@proffesor-for-testing proffesor-for-testing left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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 proffesor-for-testing left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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.

@proffesor-for-testing
proffesor-for-testing merged commit 350e627 into proffesor-for-testing:main Sep 22, 2026
22 checks passed
@proffesor-for-testing

Copy link
Copy Markdown
Owner

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 recordExperienceApplication yet (the MCP task path retrieves guidance but never feeds an outcome back), so experience_applications stays empty until source IDs are wired through task completion. Recording nothing is the right interim state, but if you want to take that wiring on next, it would be very welcome.

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