feat: harden external parent event envelopes - #108
Conversation
📝 WalkthroughWalkthroughThe PR upgrades external event logging with a versioned JSON format. ChangesEnriched external event logging
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 (2)
src/commands/send.ts (2)
199-206: 💤 Low valueConsider extracting the environment variable list to a module-level constant.
The hardcoded array of environment variable names on Line 201 could be extracted to improve maintainability, especially if this list needs to be referenced elsewhere or modified in the future.
♻️ Suggested refactor
+const HERMES_REF_ENV_VARS = ['HERMES_KANBAN_TASK', 'HERMES_KANBAN_RUN_ID', 'HERMES_KANBAN_PARENT_REF'] as const + function externalEventRefs(): Record<string, string> { const refs: Record<string, string> = {} - for (const key of ['HERMES_KANBAN_TASK', 'HERMES_KANBAN_RUN_ID', 'HERMES_KANBAN_PARENT_REF']) { + for (const key of HERMES_REF_ENV_VARS) { const value = process.env[key] if (value && value.length > 0) refs[key] = value }🤖 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/commands/send.ts` around lines 199 - 206, The array of environment variable names used inside externalEventRefs() should be pulled out to a module-level constant so it can be reused and maintained more easily; create a const (e.g., EXTERNAL_EVENT_ENV_KEYS) at top of the file containing ['HERMES_KANBAN_TASK','HERMES_KANBAN_RUN_ID','HERMES_KANBAN_PARENT_REF'] and update externalEventRefs() to iterate that constant instead of the inline array, keeping the function name and behavior unchanged.
182-192: ⚡ Quick winConsider adding a type for the event object.
The inline event object lacks type annotation, which reduces compile-time safety and IDE support. Defining an interface would catch schema errors earlier and document the expected structure.
♻️ Suggested type definition
Add an interface near the top of the file:
+interface ExternalEventPayload { + version: number + type: 'message' + from: string + to: string + text: string + message: string + ts: number + refs?: Record<string, string> +} +Then type the event object:
const refs = externalEventRefs() - const event = JSON.stringify({ + const payload: ExternalEventPayload = { version: 1, type: 'message', from, to, text: message, message, ts: Date.now(), ...(Object.keys(refs).length > 0 ? { refs } : {}), - }) + } + const event = JSON.stringify(payload)As per coding guidelines, TypeScript should be used for type safety and better developer experience.
🤖 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/commands/send.ts` around lines 182 - 192, The inline event object assigned to the variable event is untyped; add a TypeScript interface (e.g., OutgoingEvent or MessageEvent) that describes fields version, type, from, to, text, message, ts, and optional refs (matching externalEventRefs()), place the interface near the top of the file, then annotate the event variable with that interface (const event: OutgoingEvent = { ... }) so the compiler and IDE can validate the schema and catch mismatches in send.ts around the externalEventRefs() usage.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 `@src/commands/send.ts`:
- Around line 199-206: The array of environment variable names used inside
externalEventRefs() should be pulled out to a module-level constant so it can be
reused and maintained more easily; create a const (e.g.,
EXTERNAL_EVENT_ENV_KEYS) at top of the file containing
['HERMES_KANBAN_TASK','HERMES_KANBAN_RUN_ID','HERMES_KANBAN_PARENT_REF'] and
update externalEventRefs() to iterate that constant instead of the inline array,
keeping the function name and behavior unchanged.
- Around line 182-192: The inline event object assigned to the variable event is
untyped; add a TypeScript interface (e.g., OutgoingEvent or MessageEvent) that
describes fields version, type, from, to, text, message, ts, and optional refs
(matching externalEventRefs()), place the interface near the top of the file,
then annotate the event variable with that interface (const event: OutgoingEvent
= { ... }) so the compiler and IDE can validate the schema and catch mismatches
in send.ts around the externalEventRefs() usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f4d46bc7-f33d-4817-b76b-7c028f4bbad2
📒 Files selected for processing (2)
src/commands/send.tstests/unit/external-orchestrator.test.ts
1 participant
Hardens the events.jsonl envelope written by appendExternalEvent when an agent's parent is an external orchestrator (#98 follow-up):
version: 1so future envelope changes are detectable by readerstextandmessagefields for reader compatibilitytsto epoch millisecondsHERMES_KANBAN_TASK/HERMES_KANBAN_RUN_ID/HERMES_KANBAN_PARENT_REFenv refs into arefsobject when presentWithin flt nothing consumes events.jsonl (writer + sink config only); the consumer is the external hermes orchestrator this is co-designed with. Rebased on v0.3.4; tsc clean; full unit suite 838 pass / 0 fail.
Summary by CodeRabbit
Improvements
Tests