feat: Enhance training flow with wrong-answer test coverage and updat… - #148
feat: Enhance training flow with wrong-answer test coverage and updat…#148umarraza086 wants to merge 1 commit into
Conversation
SM649
left a comment
There was a problem hiding this comment.
Code Review — feat: Enhance training flow with wrong-answer test coverage
Summary: This PR improves the coaching-agent E2E test harness with three focused changes: (1) fix a response-body truncation bug that broke answer-key parsing for large training payloads, (2) add a deliberate wrong-answer injection for coverage of the error/feedback path, and (3) unlock the full baseline flow on configured accounts when baseline is incomplete.
What's good
- net-monitor truncation fix is the right call. Bumping the cap to 5 MB for
content-apiresponses (and leaving 6 KB for everything else) is a clean, targeted fix. The root-cause analysis in LEARNING.md is well-documented. correctAnswersForUnit()refactor — making it resilient to three payload shapes (per-unit object, per-unit array, bulk list) is a solid improvement. ThetryParsehelper and the ordered fallback (per-unit → bulk) are clear and safe.wrongInjectedrun-wide flag correctly guarantees at most one wrong answer per run. TheNO_WRONG=1andWRONG_PROBenv knobs are good escape hatches.- LEARNING.md is well-maintained and the open-items section is honest about what's still broken.
Issues to address
1. Hardcoded test-account credentials in SKILL.md (blocker)
- **Login email:** umar.kabaili@yopmail.com
+ **Login email:** azam.sawati@yomail.com
**Login password:** Umar@123!@#
The password Umar@123!@# is committed in plain text in SKILL.md. Even if it's a throwaway yopmail account, hardcoding credentials in source is a bad habit that could get copied to real accounts. Move credentials to env vars (e.g. CONFIGURED_EMAIL / CONFIGURED_PASSWORD) and reference them from SKILL.md with a note to set the vars in .env.
2. wrongSituation index is 1-based but sit check is also 1-based — verify the off-by-one
wrongSituation = 1 + Math.floor(Math.random() * knownCount); // 1-based
// ...
const makeWrong = sit === wrongSituation && !!correctText;sit starts at 0 and is incremented before use (sit++ in the while), so it is 1-based when evaluated in the loop body. This is correct — but the pattern is fragile. Consider naming the variable wrongSituationIndex and adding a comment confirming both are 1-based to avoid future confusion.
3. Missing newline at end of baseline-completion.json
-}
\ No newline at end of file
+}
The file still ends without a trailing newline (the diff shows \ No newline at end of file on the new entry). Minor but easy to fix and avoids noisy diffs in future PRs.
4. setupToastObserver called before login in non---fresh path — document the dependency
In baseline-agent.mjs, setupToastObserver(page) is now called before login(). If setupToastObserver assumes an authenticated page state this could silently fail. The fix looks intentional (to catch toasts during a full flow that wasn't previously triggered here), but worth a comment explaining why it's needed before login.
5. E2E confirmation still pending for the answer-key fix
LEARNING.md itself notes: "end-to-end confirmation pending the next training run (key was truncated this run, so picks were blind)." The fix is logically sound, but the PR merges code changes that haven't been verified end-to-end yet. If possible, run one training pass with the new net-monitor and attach evidence before merging to staging.
Minor nits
- The trailing space on line
+- **Login email:** azam.sawati@yomail.comin SKILL.md should be removed. WRONG_PROBdefault is 0.5 — with 6 units that means on average 3 units are candidates for the wrong-answer injection but only one wins (due towrongInjected). The probability is fine; just make sure the SKILL.md doc matches the implementation (it says "randomly chosen unit" which is accurate).
Verdict
Comment (not blocking merge, but address before next staging QA run). The core fixes are correct and the test-coverage feature is well-designed. The main ask is: (a) don't commit credentials to source, and (b) get one clean end-to-end training run as evidence before calling the answer-key fix done.
…e learning logs
Pull Request: [Feature/Fix Description]
📝 Description
Closes: [JIRA-XXXX or GitHub Issue #XXX]
🎯 Type of Change
🔍 Changes Made
🧪 E2E Testing Checklist
Required before merging to staging:
npm run devtestcoach+staging+[random]@example.comauth.userstable📸 Screenshots (if UI change)
🔄 Deployment Checklist
🚨 Breaking Changes
If checked, describe the impact:
📋 Checklist
DEVELOPMENT_STANDARDS.md)stagingbranch🔗 Related Issues
👀 Reviewers
DO NOT MERGE until:
Remember: This goes to staging first, then production after staging QA passes.