Consolidate type definitions into single source of truth#17
Consolidate type definitions into single source of truth#17
Conversation
- Merge Session, Decision, Task, and CurrentFocus types from src/context-bridge/types.ts into src/types.ts - Decision: add both date and timestamp fields, expand madeBy to include sonnet/haiku, add both reversible and confidence - Task: include dependencies/artifacts arrays, add inline execution metadata (model, duration, cost) - Session: add modelsInvolved tracking - CurrentFocus: make lastUpdated required - Add TaskResult and ContextBridgeConfig from context-bridge - Update all imports to use consolidated types - Delete redundant src/context-bridge/types.ts
|
approved |
PR Review: Consolidate type definitions into single source of truthSummaryThis PR successfully consolidates type definitions from Code Quality & Best Practices ✅Strengths:
Suggestions:
Type Definition Improvements 🎯Enhanced Decision Type:
Enhanced Task Type:
Enhanced Session Type:
Enhanced CurrentFocus Type:
Potential Issues & Bugs 🐛Issue 1: Status Value Mismatch (Minor)Location: The // types.ts
status: 'pending' | 'active' | 'blocked' | 'complete' | 'failed';
// Tests use 'pending' and 'complete' which is good
// But need to verify if 'running' was intentionally changed to 'active'Note: Looking at the old Issue 2: Timestamp Derivation Logic (Minor)Location: When parsing decisions from markdown, the code derives timestamp: new Date(date).toISOString(), // Derive from dateIf the const parsedDate = new Date(date);
timestamp: isNaN(parsedDate.getTime()) ? new Date().toISOString() : parsedDate.toISOString(),Issue 3: Missing Field in Markdown GenerationLocation: The function formatDecisionMarkdown(decision: Decision): string {
return `# ${decision.topic}
**Date:** ${decision.date}
**Made By:** ${decision.madeBy}
**Confidence:** ${decision.confidence}%
// Missing: **Reversible:** ${decision.reversible ? 'Yes' : 'No'}
...Similarly, the Performance Considerations ⚡Good:
Security Considerations 🔒Good:
Note: The filename generation in const filename = `${decision.date}-${decision.topic
.replace(/[^a-z0-9\s-]/gi, '') // Remove special chars
.replace(/\s+/g, '-')
.toLowerCase()}.md`;Test Coverage ✅Excellent:
Suggestions:
Migration Considerations 📦Important: This is a breaking change if there are existing session/task/decision files that don't have the new required fields. Recommendations:
SummaryApprove with minor suggestions. This is a well-executed refactoring that improves code organization and adds valuable metadata tracking. The main concerns are:
The type enhancements align well with Janus's multi-model architecture and will support better cost tracking, performance monitoring, and decision analysis. Recommendation: Merge after addressing the status field question and considering the migration strategy. |
There was a problem hiding this comment.
Pull request overview
This PR consolidates duplicate type definitions by merging types from src/context-bridge/types.ts into the main src/types.ts file, establishing a single source of truth for core interfaces. The consolidation enriches the existing types with additional fields from both sources and updates all imports accordingly.
Key Changes:
- Merged Session, Decision, Task, and CurrentFocus interfaces from context-bridge into main types.ts
- Enhanced Decision interface with both
timestampanddatefields, expandedmadeByoptions (sonnet/haiku), and addedreversibleandconfidencefields - Added execution metadata to Task interface (
model,duration,cost) - Made CurrentFocus
lastUpdatedfield required (was optional)
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types.ts | Consolidated type definitions from context-bridge/types.ts; added Decision fields (timestamp, date, reversible, confidence), Task execution metadata, and made CurrentFocus.lastUpdated required |
| src/context-bridge/types.ts | Deleted file - types moved to src/types.ts |
| src/context-bridge/write.ts | Updated import to use consolidated types from ../types.js |
| src/context-bridge/read.ts | Updated import to use consolidated types from ../types.js; added logic to derive timestamp from date in parseDecisionMarkdown |
| src/context-bridge/index.ts | Updated imports and exports to use consolidated types from ../types.js |
| src/context-bridge/tests/context-bridge.test.ts | Updated imports to use ../../types.js; added required fields (timestamp, reversible, dependencies, artifacts) to test objects |
| return { | ||
| id: filename.replace('.md', ''), | ||
| date, | ||
| timestamp: new Date(date).toISOString(), // Derive from date |
There was a problem hiding this comment.
Converting a date-only string to ISO timestamp assumes midnight UTC, which may not accurately reflect when the decision was made. If the timestamp information is not available in the markdown file, consider either storing only the date field or adding a comment explaining that the timestamp represents the start of that day in UTC. The current approach could lead to confusion about when decisions were actually made.
| timestamp: new Date(date).toISOString(), // Derive from date | |
| // Markdown/filename only stores a date; this timestamp represents the start of that day in UTC. | |
| timestamp: `${date}T00:00:00.000Z`, |
| import * as path from 'path'; | ||
| import { ContextBridge } from '../index'; | ||
| import { Session, Decision, Task, CurrentFocus } from '../types'; | ||
| import { Session, Decision, Task, CurrentFocus } from '../../types.js'; |
There was a problem hiding this comment.
Unused import Session.
| import { Session, Decision, Task, CurrentFocus } from '../../types.js'; | |
| import { Decision, Task, CurrentFocus } from '../../types.js'; |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| date, | ||
| timestamp: new Date(date).toISOString(), // Derive from date |
There was a problem hiding this comment.
parseDecisionMarkdown now sets timestamp from new Date(date), but date is only parts[0] from the filename (the year) while recordDecision writes filenames like YYYY-MM-DD-topic.md. That means loadDecision will always return timestamps at Jan 1 of the year (and a date of just the year), losing the actual month/day for any recorded decision. This regression appears with any decision recorded by recordDecision and then loaded via loadDecision. Consider parsing the first three filename segments (YYYY-MM-DD) before building the timestamp.
Useful? React with 👍 / 👎.
Code Review SummaryI've reviewed this PR and addressed the issues directly on main (commit 5ee8782). The type consolidation approach is correct - here's what was needed: Issues Found & Fixed
Changes Made (on main)
This PR can now be closed as the changes have been implemented directly on main. |
2 participants
src/context-bridge/types.ts into src/types.ts
sonnet/haiku, add both reversible and confidence
metadata (model, duration, cost)