feat: record review-gated project learnings - #36
Conversation
📝 WalkthroughWalkthroughThe PR adds learning events to trajectories, validates and exports their data model, introduces ChangesProject learning events
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant learningCommand
participant FileStorage
participant addLearning
participant showCommand
User->>learningCommand: trail learning summary and options
learningCommand->>FileStorage: load active trajectory
learningCommand->>addLearning: append learning
addLearning-->>learningCommand: updated trajectory
learningCommand->>FileStorage: save trajectory
User->>showCommand: trail show id --learnings
showCommand->>FileStorage: load trajectory
showCommand-->>User: render project learnings
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/trajectory.ts (1)
208-232: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPersist
validation.data, not the rawlearninginput.
addEventis built from the rawlearningobject rather thanvalidation.data. Sincez.object()strips unrecognized keys by default, any unexpected extra properties onlearningare currently smuggled intorawunchanged, and ifLearningSchemaever adds a.transform()/.default()/trim, this code would silently ignore it.♻️ Use validated data
const validation = LearningSchema.safeParse(learning); if (!validation.success) { const firstError = validation.error.issues[0]; throw new TrajectoryError( firstError.message, "VALIDATION_ERROR", "Check the learning fields and try again", ); } + const learningData = validation.data; return addEvent(trajectory, { type: "learning", - content: learning.summary, - raw: learning, + content: learningData.summary, + raw: learningData, significance: - learning.promotionStatus === "pending_review" ? "high" : "medium", + learningData.promotionStatus === "pending_review" ? "high" : "medium", tags: [ - `learning-source:${learning.source}`, - `learning-area:${learning.area}`, - `promotion:${learning.promotionStatus}`, - ...(learning.recurrenceKey - ? [`recurrence:${learning.recurrenceKey}`] + `learning-source:${learningData.source}`, + `learning-area:${learningData.area}`, + `promotion:${learningData.promotionStatus}`, + ...(learningData.recurrenceKey + ? [`recurrence:${learningData.recurrenceKey}`] : []), ], });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/trajectory.ts` around lines 208 - 232, Update the validated learning flow to use validation.data when constructing the addEvent payload, including content, raw, significance, and tags. Keep the existing validation error behavior unchanged, and ensure all persisted fields reflect the schema-normalized data rather than the original learning input.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/core/trajectory.ts`:
- Around line 208-232: Update the validated learning flow to use validation.data
when constructing the addEvent payload, including content, raw, significance,
and tags. Keep the existing validation error behavior unchanged, and ensure all
persisted fields reflect the schema-normalized data rather than the original
learning input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6d584465-6364-4281-bb7d-36ce8539c521
📒 Files selected for processing (12)
README.mdsrc/cli/commands/index.tssrc/cli/commands/learning.tssrc/cli/commands/show.tssrc/core/index.tssrc/core/schema.tssrc/core/trajectory.tssrc/core/types.d.tssrc/core/types.tssrc/index.tstests/cli/commands.test.tstests/core/trajectory.test.ts
|
Addressed the validated-data review finding in
Validation: focused 26/26 tests, full 264/264 tests, build, lint, typecheck, and |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/core/trajectory.test.ts (2)
371-378: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the specific validation error.
toThrow()could pass for an unrelated runtime failure. Assert theTrajectoryErrorvalidation contract (or its exposed error code) so this test proves the unsupported promotion status is rejected by schema validation. This follows the downstream contract insrc/core/trajectory.ts:204-234.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/core/trajectory.test.ts` around lines 371 - 378, Update the addLearning validation test in trajectory.test.ts to assert the specific TrajectoryError validation contract, or its exposed error code, rather than only checking that some error is thrown. Ensure the assertion verifies that the unsupported promotionStatus value is rejected by schema validation.
340-343: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the complete persisted learning payload.
The current partial assertions can pass even if valid fields such as
summary,source, orareaare dropped. Compareevent.rawwith the exact schema-normalized object while also verifying thatunexpectedis absent. The implementation persistsvalidation.datadirectly asraw(src/core/trajectory.ts:204-234).Also applies to: 361-363
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/core/trajectory.test.ts` around lines 340 - 343, Update the assertions around the persisted learning event in the trajectory test to compare event.raw against the complete schema-normalized validation payload, including fields such as summary, source, area, promotionStatus, and recurrenceKey. Explicitly verify that the unexpected field is absent, while preserving the existing persisted validation.data behavior in the trajectory implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/core/trajectory.test.ts`:
- Around line 371-378: Update the addLearning validation test in
trajectory.test.ts to assert the specific TrajectoryError validation contract,
or its exposed error code, rather than only checking that some error is thrown.
Ensure the assertion verifies that the unsupported promotionStatus value is
rejected by schema validation.
- Around line 340-343: Update the assertions around the persisted learning event
in the trajectory test to compare event.raw against the complete
schema-normalized validation payload, including fields such as summary, source,
area, promotionStatus, and recurrenceKey. Explicitly verify that the unexpected
field is absent, while preserving the existing persisted validation.data
behavior in the trajectory implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e93bea1c-c570-4e40-9a96-bfbc5370ae84
📒 Files selected for processing (2)
src/core/trajectory.tstests/core/trajectory.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/trajectory.ts
|
Addressed the two latest test-quality suggestions in
Validation is clean: focused tests 26/26, full suite 264/264, build, lint, typecheck, and |
|
@willwashburn, when you have a moment, could you please review this contribution? The latest automated feedback has been addressed and all local validation gates are passing. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/core/trajectory.test.ts (1)
323-326: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign all new trajectory test setup with the construction guideline.
All three tests use
createTrajectoryfor in-memory trajectory construction. UseTrajectoryBuilder, or document an explicit exemption for core trajectory unit tests.
tests/core/trajectory.test.ts#L323-L326: Replace thecreateTrajectorysetup withTrajectoryBuilder.tests/core/trajectory.test.ts#L352-L355: Replace thecreateTrajectorysetup withTrajectoryBuilder.tests/core/trajectory.test.ts#L376-L379: Replace thecreateTrajectorysetup withTrajectoryBuilder.As per coding guidelines:
**/*.{ts,tsx}: Use the TrajectoryClient for persistent storage and TrajectoryBuilder for in-memory construction when programmatically creating or managing agent trajectories in TypeScript.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/core/trajectory.test.ts` around lines 323 - 326, Update the trajectory setup in tests/core/trajectory.test.ts at lines 323-326, 352-355, and 376-379 to construct in-memory trajectories with TrajectoryBuilder instead of createTrajectory; update imports and preserve each test’s existing behavior and configuration.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/core/trajectory.test.ts`:
- Around line 323-326: Update the trajectory setup in
tests/core/trajectory.test.ts at lines 323-326, 352-355, and 376-379 to
construct in-memory trajectories with TrajectoryBuilder instead of
createTrajectory; update imports and preserve each test’s existing behavior and
configuration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 12a79ea7-f013-4912-b3e4-90b599ad8ea9
📒 Files selected for processing (1)
tests/core/trajectory.test.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 12 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/cli/commands.test.ts">
<violation number="1" location="tests/cli/commands.test.ts:267">
P3: This negative test only asserts result.success is false and never verifies it failed for the intended reason. If the source validation were accidentally removed (or some unrelated error occurred), the test would still pass, so it does not guard the validation behavior it claims to cover. Consider asserting on result.error (e.g., that it mentions the unsupported source value).</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| "src", | ||
| ]); | ||
|
|
||
| expect(result.success).toBe(false); |
There was a problem hiding this comment.
P3: This negative test only asserts result.success is false and never verifies it failed for the intended reason. If the source validation were accidentally removed (or some unrelated error occurred), the test would still pass, so it does not guard the validation behavior it claims to cover. Consider asserting on result.error (e.g., that it mentions the unsupported source value).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/cli/commands.test.ts, line 267:
<comment>This negative test only asserts result.success is false and never verifies it failed for the intended reason. If the source validation were accidentally removed (or some unrelated error occurred), the test would still pass, so it does not guard the validation behavior it claims to cover. Consider asserting on result.error (e.g., that it mentions the unsupported source value).</comment>
<file context>
@@ -176,6 +176,98 @@ describe("CLI Commands", () => {
+ "src",
+ ]);
+
+ expect(result.success).toBe(false);
+ });
+ });
</file context>
| expect(result.success).toBe(false); | |
| expect(result.success).toBe(false); | |
| expect(result.error).toContain("source"); |
1 participant
Summary
Adds the first mergeable slice of #31: structured project learnings that remain separate from decisions, reflections, and durable project instructions.
learningtrajectory event with source, affected area, evidence, recurrence key, and promotion status.trail learningfor recording one-off learnings or human-review candidates.trail show <id> --learningsfor querying them separately.Safety boundary
This slice deliberately does not write to
AGENTS.md,CLAUDE.md, or skills. One-off learnings arearchived;--promotion-candidaterecordspending_review. There is no approved state in this PR, so a candidate cannot be mistaken for a human-approved durable instruction.Recurrence aggregation, configurable thresholds, reviewer authentication, and promotion adapters remain follow-up work.
Validation
npm test -- --run— 263 tests passednpm run buildnpm run lintnpm run typecheckAI assistance
Implemented with OpenAI Codex assistance and reviewed by SuhaibAslam before submission.