feat: web harness improvements - #151
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
sergiofilhowz
marked this pull request as ready for review
May 18, 2026 12:43
Contributor
skill-check — worker0 verified, 26 skipped (no docs/).
Three for three. Nicely done. |
📝 WalkthroughWalkthroughThis PR transfers system-prompt construction from the console to the harness and extends it with mode-specific prefixes. It clarifies turn boundaries via explicit ChangesMode-aware System Prompts and Message Boundary Handling
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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.
Actionable comments posted: 2
🧹 Nitpick comments (2)
harness-node/src/turn-orchestrator/states/provisioning.ts (1)
9-11: ⚡ Quick winCentralize mode validation to prevent drift.
asModeduplicates the same mode guard logic already present insystem-prompt.ts. If modes change later, these checks can silently diverge. Export a single validator and reuse it here.Suggested refactor
- function asMode(value: unknown): Mode | null { - return value === 'plan' || value === 'ask' || value === 'agent' ? value : null; - } +// system-prompt.ts +export function isMode(value: unknown): value is Mode { + return value === 'plan' || value === 'ask' || value === 'agent' +}- import { type DefaultSkillBody, type Mode, buildSystemPrompt, defaultSkillBody } from '../system-prompt.js'; + import { type DefaultSkillBody, buildSystemPrompt, defaultSkillBody, isMode } from '../system-prompt.js'; - const mode = asMode(request.mode); + const mode = isMode(request.mode) ? request.mode : null;🤖 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 `@harness-node/src/turn-orchestrator/states/provisioning.ts` around lines 9 - 11, Replace the duplicated mode-guard in provisioning.ts by importing and reusing the centralized validator from system-prompt.ts: export the canonical mode validator (e.g., isMode or parseMode) from system-prompt.ts and then change the local asMode function in provisioning.ts to call that exported validator (or remove asMode and directly use the imported validator), ensuring all mode checks reference the single shared function to avoid drift.harness-node/src/turn-orchestrator/transitions.ts (1)
11-11: ⚡ Quick winUse structured logging instead of console.log for production observability.
The
console.logstatement should use a proper logger (similar to theloggerused instream.ts) for structured logging, appropriate log levels, and better observability in production environments.📊 Proposed improvement: Import and use logger
Add logger import:
+import { logger } from '../runtime/otel.js' import type { ISdk } from '../runtime/iii.js'Replace console.log:
export async function step(iii: ISdk, cfg: TurnOrchestratorConfig, rec: TurnStateRecord): Promise<void> { - console.log('step transition from', rec.state) + logger.debug('step transition', { from: rec.state }) switch (rec.state) {🤖 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 `@harness-node/src/turn-orchestrator/transitions.ts` at line 11, Replace the console.log in transitions.ts with the project's structured logger: import the same logger used in stream.ts (e.g., `import { logger } from '...stream'` or the shared logger module) and change the call `console.log('step transition from', rec.state)` to a structured log like `logger.info({ event: 'step_transition', fromState: rec.state, recordId: rec.id })` (or use `logger.debug` if lower verbosity is desired); ensure the import is added at top and any payload includes relevant fields (rec.state and an identifier) for observability.
🤖 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.
Inline comments:
In `@harness-node/src/provider-anthropic/stream.ts`:
- Around line 49-50: Remove the two console.log calls that print headers and
body (they leak authHeaderFor(cfg) and message content); instead, delete those
lines in stream.ts and, if diagnostic info is required, use the existing logger
(imported as logger) and log non-sensitive fields only or redact authorization
and prompt/message content before logging. Ensure references to headers, body,
and authHeaderFor(cfg) are not directly printed to stdout.
In `@harness-node/src/turn-orchestrator/states/functions.ts`:
- Around line 94-99: The current code starts persistence.saveExecutedCalls(...)
into savePromise then awaits emit(...), but if emit throws the savePromise is
never awaited; change the flow so savePromise is always awaited (either use
Promise.all([savePromise, emitPromise]) or wrap the emit call in try/finally and
await savePromise in the finally block) to ensure
persistence.saveExecutedCalls(iii, rec.session_id, results) is always awaited
and any errors are surfaced; adjust error handling so failures from both
emit(...) and persistence.saveExecutedCalls(...) are propagated or logged
appropriately.
---
Nitpick comments:
In `@harness-node/src/turn-orchestrator/states/provisioning.ts`:
- Around line 9-11: Replace the duplicated mode-guard in provisioning.ts by
importing and reusing the centralized validator from system-prompt.ts: export
the canonical mode validator (e.g., isMode or parseMode) from system-prompt.ts
and then change the local asMode function in provisioning.ts to call that
exported validator (or remove asMode and directly use the imported validator),
ensuring all mode checks reference the single shared function to avoid drift.
In `@harness-node/src/turn-orchestrator/transitions.ts`:
- Line 11: Replace the console.log in transitions.ts with the project's
structured logger: import the same logger used in stream.ts (e.g., `import {
logger } from '...stream'` or the shared logger module) and change the call
`console.log('step transition from', rec.state)` to a structured log like
`logger.info({ event: 'step_transition', fromState: rec.state, recordId: rec.id
})` (or use `logger.debug` if lower verbosity is desired); ensure the import is
added at top and any payload includes relevant fields (rec.state and an
identifier) for observability.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 46c69c21-aed4-47d2-b5eb-38a13596edfa
📒 Files selected for processing (13)
console/web/src/components/chat/ChatView.tsxconsole/web/src/components/chat/FunctionCallMessage.tsxconsole/web/src/components/chat/MessageList.tsxconsole/web/src/lib/backend/real.tsconsole/web/src/lib/backend/translate.tsharness-node/src/provider-anthropic/auth.tsharness-node/src/provider-anthropic/stream.tsharness-node/src/turn-orchestrator/run-start.tsharness-node/src/turn-orchestrator/states/functions.tsharness-node/src/turn-orchestrator/states/provisioning.tsharness-node/src/turn-orchestrator/system-prompt.tsharness-node/src/turn-orchestrator/transitions.tsharness-node/tests/turn-orchestrator/system-prompt.test.ts
Comment on lines
+49
to
+50
| console.log('headers', headers) | ||
| console.log('body', body) |
There was a problem hiding this comment.
Remove debug logging that exposes credentials.
These console.log statements expose sensitive authentication headers and potentially sensitive prompt/message content. The headers object includes the authorization credential from authHeaderFor(cfg) (lines 42-47), and logging it to console risks leaking API keys in production logs.
Remove these debug statements or, if diagnostic logging is needed, use the existing logger (imported at line 7) with proper redaction of sensitive fields.
🔒 Proposed fix: Remove debug logging
- console.log('headers', headers)
- console.log('body', body)
-
const ac = new AbortController()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| console.log('headers', headers) | |
| console.log('body', body) | |
| const ac = new AbortController() |
🤖 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 `@harness-node/src/provider-anthropic/stream.ts` around lines 49 - 50, Remove
the two console.log calls that print headers and body (they leak
authHeaderFor(cfg) and message content); instead, delete those lines in
stream.ts and, if diagnostic info is required, use the existing logger (imported
as logger) and log non-sensitive fields only or redact authorization and
prompt/message content before logging. Ensure references to headers, body, and
authHeaderFor(cfg) are not directly printed to stdout.
Comment on lines
+94
to
+99
| // Kick off persistence in parallel with the user-facing emit so the UI's | ||
| // fcall-end lands ~one trigger round-trip sooner. We still await both | ||
| // before the next iteration so ordering and durability are preserved. | ||
| const savePromise = persistence.saveExecutedCalls(iii, rec.session_id, results); | ||
| await emit(iii, rec.session_id, buildFunctionExecutionEnd(fc, result, is_error)); | ||
| await savePromise; |
There was a problem hiding this comment.
Ensure savePromise is always awaited to prevent unhandled rejections.
If emit (line 98) throws an exception, the savePromise (line 97) is abandoned without being awaited. This can result in:
- Unhandled promise rejections if the save operation fails
- Loss of error information from the persistence layer
- Potential Node.js process warnings or crashes in strict rejection mode
Use Promise.all or wrap in try/finally to guarantee the save promise is awaited even if emit fails.
🛡️ Proposed fix: Ensure both operations are awaited
- const savePromise = persistence.saveExecutedCalls(iii, rec.session_id, results);
- await emit(iii, rec.session_id, buildFunctionExecutionEnd(fc, result, is_error));
- await savePromise;
+ // Kick off persistence in parallel with the user-facing emit so the UI's
+ // fcall-end lands ~one trigger round-trip sooner. We still await both
+ // before the next iteration so ordering and durability are preserved.
+ await Promise.all([
+ persistence.saveExecutedCalls(iii, rec.session_id, results),
+ emit(iii, rec.session_id, buildFunctionExecutionEnd(fc, result, is_error)),
+ ]);🤖 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 `@harness-node/src/turn-orchestrator/states/functions.ts` around lines 94 - 99,
The current code starts persistence.saveExecutedCalls(...) into savePromise then
awaits emit(...), but if emit throws the savePromise is never awaited; change
the flow so savePromise is always awaited (either use Promise.all([savePromise,
emitPromise]) or wrap the emit call in try/finally and await savePromise in the
finally block) to ensure persistence.saveExecutedCalls(iii, rec.session_id,
results) is always awaited and any errors are surfaced; adjust error handling so
failures from both emit(...) and persistence.saveExecutedCalls(...) are
propagated or logged appropriately.
This was referenced May 18, 2026
Merged
5 tasks
This was referenced Jun 10, 2026
5 tasks
This was referenced Jul 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation