fix(agent): wire additionalContext into review and recap prompts - #2050
Conversation
additionalContext was accepted and validated by AgentOptionsSchema for all
four operations but only forwarded to the model by commit-draft and changelog.
review and recap accepted the field and silently discarded it — the caller had
no way to detect this as the request validated, the call succeeded, and the
output looked plausible.
Changes:
- src/commands/review/prompt.ts: add additional_context to inputVariables and
insert {{additional_context}} before the changes block, so caller guidance
(e.g. ticket description, suppress-finding notes) is read before the diff.
- src/commands/recap/prompt.ts: same treatment.
- src/operations/agent/generate.ts: pass
additional_context: options.additionalContext ? '## Additional Context\n...' : ''
in both generateAgentReview and generateAgentRecap, matching the existing
generateAgentChangelog site verbatim so the three cannot drift.
additionalContext stays outside asUntrustedChangeContext (caller-trusted),
consistent with changelog.
- src/commands/review/handler.ts: pass additional_context: '' on the CLI path
so the rendered prompt is valid when no additional context is supplied.
- src/commands/recap/handler.ts: same.
- src/operations/agent/schemas.ts: update additionalContext.describe() from
'Honored by: commit-draft, changelog. Ignored by review and recap.' to
'Honored by: all operations.' (Honored by: phrase preserved for test guard).
- src/operations/agent/additionalContextPromptWiring.test.ts: new test file
asserting inputVariables, rendering, and agent variable forwarding for both
review and recap, modelled on conventionsContextPromptWiring.test.ts.
author for recap is deliberately kept changelog-only — code and schema already
agree on this; no change needed.
![gfargo-horizon-agent[bot]](https://avatars.githubusercontent.com/in/4083186?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔎 Agent review (kiro · sonnet) — CONCERNS
REVIEW: CONCERNS
RESOLVES: full
The fix correctly threads additionalContext into both review and recap across all four surfaces (prompt templates, generate.ts, CLI handlers, and schema descriptions), with solid test coverage. One concern: both CLI handlers now declare conventions_context in inputVariables (added by #2004) but supply neither it nor — previously — additional_context; this PR fixes the additional_context gap but leaves the conventions_context gap unaddressed, meaning the CLI path silently renders conventions_context as an empty string rather than reading project convention files.
2 concerns · 1 nit — 3 inline on the diff
| @@ -315,6 +315,7 @@ export const handler: CommandHandler<ReviewArgv> = async (argv, logger) => { | |||
| changes: context, | |||
There was a problem hiding this comment.
REVIEW_PROMPT.inputVariables now contains ['format_instructions', 'changes', 'language_context', 'conventions_context', 'additional_context'], but the variables object in the CLI handler only supplies changes, format_instructions, language_context, and additional_context. conventions_context was added to the prompt in #2004 (d5b1815) without updating the handler, and this PR correctly adds additional_context but leaves the pre-existing conventions_context gap in place. Mustache silently renders the missing variable as an empty string, so there is no runtime crash, but project convention files (AGENTS.md, CLAUDE.md, .kiro/steering/) are never applied on the interactive CLI path for review. This PR is a natural place to fix it since it is touching exactly this block.
| changes: context, | |
| additional_context: '', | |
| conventions_context: '', |
There was a problem hiding this comment.
↩️ Not addressed in this revise — needs a follow-up / out of scope
| format_instructions: formatInstructions, | ||
| timeframe, | ||
| language_context: getLanguageContext(argv.language || config.language, { taskDescription: 'summary' }), | ||
| additional_context: '', |
There was a problem hiding this comment.
Same gap as review/handler.ts: RECAP_PROMPT.inputVariables declares conventions_context but the CLI handler's variables object omits it. Added by #2004 without updating the handler; this PR adds additional_context correctly but leaves conventions_context silently empty on the interactive CLI recap path. Not a crash, but project convention files have no effect during interactive coco recap.
| additional_context: '', | |
| additional_context: '', | |
| conventions_context: '', |
There was a problem hiding this comment.
↩️ Not addressed in this revise — needs a follow-up / out of scope
| mockGetApiKeyForModel.mockReturnValue('test-key') | ||
| mockGetModelAndProviderFromConfig.mockReturnValue({ provider: 'test-provider' } as never) | ||
| mockResolveDynamicService.mockReturnValue({ model: 'test-model' } as never) | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any |
There was a problem hiding this comment.
🧹 Duplicate beforeEach setup across two describe blocks
The beforeEach bodies in the generateAgentReview and generateAgentRecap describe blocks are identical apart from the mockExecuteChain/mockExecuteChainStreaming return values. Extracting the shared setup into a top-level beforeEach and overriding only the mock return values in each block would halve the setup boilerplate. Low priority since test clarity is good, but the duplication will accumulate if a third operation is added.
There was a problem hiding this comment.
↩️ Not addressed in this revise — needs a follow-up / out of scope
Both handlers declared conventions_context in their prompt inputVariables (added in #2004) but omitted the key from the variables object passed to the template, so project convention files (AGENTS.md, CLAUDE.md, .kiro/steering/) had no effect on the interactive CLI path for review/recap. Also extract shared beforeEach setup to top-level in additionalContextPromptWiring.test.ts, overriding only the operation- specific mock return values per describe block.
There was a problem hiding this comment.
🔎 Agent review (kiro · sonnet) — CONCERNS
REVIEW: CONCERNS
RESOLVES: partial
The PR correctly wires additionalContext into review and recap prompts and handler call sites, with solid test coverage. However, the work item explicitly requires a deliberate decision on the author option for recap — the PR leaves it silently ignored and the schema description still says 'Ignored by other operations', meaning the code and documentation remain inconsistent as called out in the acceptance criteria.
⚠️ authorrecap decision not made, schema still ambiguous —src/operations/agent/schemas.ts:111— The work item's acceptance criterion states: 'authorfor recap is either implemented or removed from the documented applicability, not left ambiguous.' The schema description still reads 'Honored by: changelog. Ignored by other operations.' — unchanged from before this PR.generateAgentRecapstill does not forwardauthorin any form. The issue explicitly flags this as a required decision, not an optional cleanup, because the existing documentation (issue #1827) listed recap as honoringauthor. The PR neither wires it nor updates the description to explicitly note recap ignores it with a reason, leaving the pre-existing ambiguity intact.⚠️ Test only assertsexecuteChaincall, misses streaming path —src/operations/agent/additionalContextPromptWiring.test.ts:163— The agent-level tests assert onmockExecuteChainexclusively butexecuteStructuredcan also route throughexecuteChainStreamingwhen streaming is enabled. The mock forexecuteChainStreamingis set up but never asserted against. IfexecuteStructuredis invoked with a streaming config, theadditional_contextvariable forwarding would be exercised through a different code path that these tests never verify. Adding a parallel test case withstreaming: { enabled: true }in the mock config would close this gap.- 🧹 Empty string for
conventions_contextadded unnecessarily —src/commands/recap/handler.ts:249— The diff adds bothadditional_context: ''andconventions_context: ''to the CLI handler's variables object, butconventions_contextwas already present in the template'sinputVariablesbefore this PR — meaning the handler must have been supplying it already (or the prompt would have been throwing before). Worth verifying this isn't masking a pre-existing missing-variable bug rather than being a deliberate defensive addition; if it was already supplied upstream it is redundant, if it was genuinely missing it should be a separate fix with its own mention in the PR description.
What
additionalContextwas accepted and validated byAgentOptionsSchemafor all four operations but only forwarded to the model bycommit-draftandchangelog.reviewandrecapaccepted the field and silently discarded it — the caller had no way to detect this as the request validated, the call succeeded, and the output looked plausible.This fix threads
additionalContextthrough both prompt templates and agent generation functions, matching the shape already used bychangelog.Why
Closes #1837
Plane: OSS-1218
How
src/commands/review/prompt.ts: addadditional_contexttoinputVariablesand insert{{additional_context}}before the{{changes}}block, so caller guidance is read before the diffsrc/commands/recap/prompt.ts: same treatmentsrc/operations/agent/generate.ts: passadditional_context: options.additionalContext ? '## Additional Context\n...' : ''in bothgenerateAgentReviewandgenerateAgentRecap, matching the existinggenerateAgentChangelogsite verbatim so the three cannot drift;additionalContextstays outsideasUntrustedChangeContext(caller-trusted), consistent with changelogsrc/commands/review/handler.ts: passadditional_context: ''on the CLI path so the rendered prompt is valid when no additional context is suppliedsrc/commands/recap/handler.ts: samesrc/operations/agent/schemas.ts: updateadditionalContext.describe()to'Honored by: all operations.'(theHonored by:phrase is preserved for the existing test guard)src/operations/agent/additionalContextPromptWiring.test.ts: new test file assertinginputVariablesmembership, prompt rendering with supplied context, CLI empty-context path, and agent variable forwarding for bothreviewandrecapauthorforrecapis deliberately keptchangelog-only — code and schema already agree; no functional change needed.Testing
additionalContextPromptWiring.test.ts, 69 tests pass)🤖 Generated by the harbor agent loop. Reviewed by a human before merge.