fix(certification): show demo params and result before discussion in all Sage demo turns#4944
Open
bokelley wants to merge 1 commit into
Open
fix(certification): show demo params and result before discussion in all Sage demo turns#4944bokelley wants to merge 1 commit into
bokelley wants to merge 1 commit into
Conversation
… sites Issue #4940: Sage was discussing demo tool results without first rendering them to the learner, violating the teaching rule "NEVER reference content you haven't shown." Three prompt-string injection sites in certification-tools.ts now mandate a TWO-STEP SEQUENCE: (1) announce request parameters in plain language before the tool call; (2) paste the full result verbatim in the learner-facing message before any interpretive text. Applies to buildCertificationContext (start path), the start_ certification_module demo block, and the get_certification_module live-demos reload block. https://claude.ai/code/session_01KNxrd3yCz6idFsLrRJmhBx
Contributor
There was a problem hiding this comment.
LGTM. Follow-ups noted below. Three-site coverage is right — the trimmed-session reload path at L1749 was the load-bearing catch; without it the bug reproduces immediately after context truncation.
Things I checked
- All three sites carry compatible TWO-STEP SEQUENCE language with matching escape clauses (
certification-tools.ts:641,:1606,:1749). The verbatim-or-excerpt-with-"…" wording is identical across sites. - The new rule does not contradict the pre-existing "NEVER reference content you haven't shown" at
:645; site:1606explicitly cites it as the rule being enforced for demo turns. - Changeset
.changeset/fix-a2-demo-result-visibility.mduses the empty---\n---frontmatter, which matches the established convention in this repo for non-package-bumping changes (sampled three recent.changeset/fix-*.mdfiles). - 150-word cap tension is acknowledged in-prompt at all three sites with "show the result first and discuss it next turn." The two-turn demo pattern is now the documented expectation.
- No schema, auth, migration, or wire surface touched. Pure runtime prompt content.
Follow-ups (non-blocking — file as issues)
- Fourth surface drift.
TEACHING_METHODOLOGYatcertification-tools.ts:156-226(appended vialines.push(TEACHING_METHODOLOGY)at:1809) still carries the older "describe what the result would look like" framing without the show-params-first sequence. Not a contradiction — TEACHING_METHODOLOGY is general principle, the three patched sites are turn-mechanics — but it's now the weakest link, and a future editor of the canonical methodology constant won't see they need to mirror three other sites. Worth folding the two-step rule into TEACHING_METHODOLOGY as the single source and having:641/:1606/:1749reference it by name. Three near-duplicate paragraphs is exactly how drift starts. - Over-application to capstone and build-project tracks. The injections at
:1606and:1749fire onget_certification_moduleregardless of module track.CAPSTONE_METHODOLOGYat:233-245does not carry the "NEVER reference content you haven't shown" rule because Sage at capstone is assessing, not demonstrating — but a capstone learner running a lab phase now gets a mandatory two-step demo instruction anyway.BUILD_PROJECT_METHODOLOGYalready says "present the tool's response exactly as returned" at:110, so the new rule is redundant there. TheisBuildProjectguard pattern at:1804-1807is the model to mirror. - Disengaged-learner re-engagement exception. The modality at
:203prescribes "run a demo" as the re-engagement move. A disengaged learner getting a turn of "I'm about to query X," then a turn of pasted JSON, then a turn of discussion is more disengaged, not less. One sentence exception: when running a demo to re-engage a checked-out learner, params and result may collapse into one message provided params still appear before result.
Minor nits (non-blocking)
- PR body misdescribes site 1747. Body calls it the "context-reload path"; reading the handler,
handlers.set('start_certification_module', ...)at:1656puts this site inside the start-module handler's lesson-plan formatter, not a reload path. Code change at that site is correct — only the description is off. - Redundant escape clause inside site 641. Line
:645already states the show-first-discuss-next-turn rule. Site:641restating it inside the same emitted prompt is mildly redundant. No functional risk; reinforcement is the likely intent.
Notable choice: PR title bills this as "all three Sage demo turns" while TEACHING_METHODOLOGY quietly remains a fourth surface with the older wording. Worth a follow-up before the next learner-feedback cycle.
Approving on the strength of the three-site coverage plus the explicit changeset.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #4940
During the A2 "Your first media buy" certification module, Sage was calling
get_products(and other demo tools) and then discussing the results without first rendering them to the learner — violating the teaching rule "NEVER reference content you haven't shown." The learner also couldn't see the request parameters (brief text, brand domain) before the call fired. This PR patches all three prompt-string injection sites incertification-tools.tsthat emit demo instructions to Sage.Non-breaking justification: purely modifies string constants injected into Sage's LLM context at runtime; no protocol schema changes, no TypeScript interface changes, no database migrations, no assessment criteria or module IDs changed. Existing credentials are unaffected.
Changes:
buildCertificationContext(line 641) —start_certification_modulepath: replaces "Show, then discuss" with an explicit TWO-STEP SEQUENCE requiring Sage to (1) state request parameters before the tool call and (2) paste the full result verbatim before any interpretive text.createCertificationToolHandlersdemo-scenarios block (line 1606) —get_certification_modulepath: same two-step mandate added after the "Let me show you..." anchor phrase; also adds the explicit escape hatch for the 150-word cap.createCertificationToolHandlerslive-demos block (line 1747) —get_certification_modulecontext-reload path: new injection; previously had no sequencing instruction, meaning trimmed-session reloads could reproduce the bug immediately.Pre-PR review:
TWO-STEP SEQUENCE (mandatory):across all three sites, "summarise" → "reference", "full" added to site-1606, changeset body updated to cover all demo tools)Note on build errors: The repo has 4892 pre-existing TypeScript errors on
main(missing@types/nodeenvironment configuration). My diff introduces zero new errors — confirmed bynpx tsc --noEmit 2>&1 | grep certification-toolsreturning empty.Session: https://claude.ai/code/session_01KNxrd3yCz6idFsLrRJmhBx
Generated by Claude Code