[replay #7505] test(agent-service): pin the context serializer and the execution tools - #16
Open
sshiv012 wants to merge 2 commits into
Open
[replay #7505] test(agent-service): pin the context serializer and the execution tools#16sshiv012 wants to merge 2 commits into
sshiv012 wants to merge 2 commits into
Conversation
context-utils.ts builds the prompt the agent reasons over, and workflow-execution-tools.ts turns an execution result into the table it reads back. Both were partly covered by line count and thinly covered in substance: bun credits a whole function body once entered, so blocks inside jsonToTableFormat and executeOperatorAndFormat counted as covered while nothing asserted them. Adds 15 tests across the two existing specs. context-utils.ts reaches 100% lines and functions; workflow-execution-tools.ts goes from 91.24% to 95.16%, with the remainder left alone because it is dead. Ten of the mutations these kill target lines bun already reported as covered - the DAG topological ordering and its tie-break, port ordinal mapping, the NULL/null/undefined/object cell rendering, the row-gap ellipsis, and the schema-violation messages. No production file is touched.
📝 WalkthroughWalkthroughThe pull request expands tests for workflow execution tools and context utilities. Coverage includes request construction, validation, result rendering, workflow serialization, property handling, status reporting, and compilation errors. ChangesWorkflow test coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![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
🤖 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 `@agent-service/src/agent/tools/workflow-execution-tools.spec.ts`:
- Around line 330-365: Update the ordinal-resolution test around
executeOperatorAndFormat to use a declared source port instead of output-9, and
adjust its expected link accordingly. Add a separate test covering an unknown
port on a non-dynamic operator, asserting execution is blocked and fetch is not
called.
In `@agent-service/src/agent/util/context-utils.spec.ts`:
- Around line 206-219: Update serializeOperator and the assembleContext flow so
useRedact=true never includes operatorProperties, including for [ERROR]
execution results. Preserve the failed status and expose only non-sensitive
error details in the returned ModelMessage; update the affected test
expectations accordingly.
🪄 Autofix
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 Plus
Run ID: 75eaf20b-1ac7-4407-8c1f-8e8ddeae28f9
📒 Files selected for processing (2)
agent-service/src/agent/tools/workflow-execution-tools.spec.tsagent-service/src/agent/util/context-utils.spec.ts
Comment on lines
+330
to
+365
| state.addLink(makeLink("src", "output-1", "dst", "input-0")); | ||
| // An unknown source port falls back to ordinal 0 rather than -1. | ||
| state.addLink(makeLink("src", "output-9", "dst", "input-1")); | ||
| resolveFetch(fetchSpy, { | ||
| success: true, | ||
| state: "Completed", | ||
| operators: { | ||
| dst: { state: "Completed", inputTuples: 0, outputTuples: 1, resultMode: "table", result: [{ a: 1 }] }, | ||
| }, | ||
| }); | ||
|
|
||
| await executeOperatorAndFormat(state, cfg({ workflowId: 7, computingUnitId: 3, executionTimeoutMs: 4200 }), "dst"); | ||
|
|
||
| expect(String(fetchSpy.mock.calls[0][0])).toBe("http://localhost:8085/api/execution/7/3/run"); | ||
| expect((fetchSpy.mock.calls[0][1] as RequestInit).headers).toMatchObject({ Authorization: "Bearer tok" }); | ||
|
|
||
| const body = requestBody(fetchSpy); | ||
| expect(body.executionName).toBe("agent-execution"); | ||
| // 4200 rather than a round 4500: with 4500 the assertion holds for ceil, round *and* | ||
| // floor-plus-one, so it would not notice the rounding being changed. 4200 separates them. | ||
| expect(body.timeoutSeconds).toBe(5); | ||
| expect(body.targetOperatorIds).toEqual(["dst"]); | ||
| expect(body.logicalPlan.opsToViewResult).toEqual(["dst"]); | ||
| expect(body.logicalPlan.links).toEqual([ | ||
| { | ||
| fromOpId: "src", | ||
| fromPortId: { id: 1, internal: false }, | ||
| toOpId: "dst", | ||
| toPortId: { id: 0, internal: false }, | ||
| }, | ||
| { | ||
| fromOpId: "src", | ||
| fromPortId: { id: 0, internal: false }, | ||
| toOpId: "dst", | ||
| toPortId: { id: 1, internal: false }, | ||
| }, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Reject an unknown static source port instead of mapping it to port 0.
src declares only output-0 and output-1. Line 332 creates output-9, but Lines 360-365 assert that it becomes ordinal 0. This changes an invalid link into a valid-looking link from the wrong output port.
Use a declared port in this ordinal-resolution test. Add a separate test that an unknown port on a non-dynamic operator blocks execution without calling fetch.
🤖 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 `@agent-service/src/agent/tools/workflow-execution-tools.spec.ts` around lines
330 - 365, Update the ordinal-resolution test around executeOperatorAndFormat to
use a declared source port instead of output-9, and adjust its expected link
accordingly. Add a separate test covering an unknown port on a non-dynamic
operator, asserting execution is blocked and fetch is not called.
Comment on lines
+206
to
+219
| test("omits properties under redaction unless the operator's result reports an error", () => { | ||
| const workflowState = new WorkflowState(); | ||
| workflowState.addOperator(makeOperator("op1", { operatorProperties: { secret: "s3cr3t" } })); | ||
|
|
||
| const redacted = contentOf(assembleContext([], workflowState, new Map(), true)); | ||
| expect(redacted).toContain("(TestOp, not-executed)"); | ||
| expect(redacted).not.toContain("Properties:"); | ||
|
|
||
| // A failing operator keeps its properties even when redacting, so the model | ||
| // can see what caused the failure. | ||
| const failed = contentOf(assembleContext([], workflowState, new Map([["op1", "[ERROR] boom"]]), true)); | ||
| expect(failed).toContain("(TestOp, failed)"); | ||
| expect(failed).toContain(" secret: s3cr3t"); | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline agent-service/src/agent/util/context-utils.ts --items all
rg -n -C 8 'function serializeDag|useRedact|operatorProperties|operatorExecutionResults|Result:' \
agent-service/src/agent/util/context-utils.ts \
agent-service/src/agent/util/context-utils.spec.tsRepository: sshiv012/texera
Length of output: 21291
Sensitive Data Exposure (CWE-200): Exposure of Sensitive Information to an Unauthorized Actor
Keep operator properties redacted when useRedact is true.
When an execution result contains [ERROR], serializeOperator includes all operatorProperties in the returned ModelMessage. Preserve redaction and include only non-sensitive error details.
🤖 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 `@agent-service/src/agent/util/context-utils.spec.ts` around lines 206 - 219,
Update serializeOperator and the assembleContext flow so useRedact=true never
includes operatorProperties, including for [ERROR] execution results. Preserve
the failed status and expose only non-sensitive error details in the returned
ModelMessage; update the affected test expectations accordingly.
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
3 participants
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.
Replay of apache#7505 for CodeRabbit evaluation.
merge-base
42d08a3701cdheadd0d9cb1447e8stratumfrontendSynthetic evaluation PR. Do not merge.
context-utils.tscoverage to 100%.workflow-execution-tools.tscoverage to 95.16%.