refactor(agent): give the agent loop's skill policy one owner - #3440
Conversation
Give the request-scoped active-skill policy one owner: a mutable per-attempt class holding activeSkillId/Policy/ToolAvailability/ DelegationOverrides and hasSubmittedFormInput, hydrated from replay history and mutated via applySuccessfulResult/markFormInputSubmitted. Not yet wired into the loops (next commit); this only adds the module and its tests, which cover hydration, both transitions, and that two instances never share state.
Replace the duplicated 18-line skill-policy prologue and the removeFormInputAfterSubmission + flag pair (4 call sites: 1 in executeAgentLoop, 3 in executeAgentLoopStreaming) with reads/writes on one AgentLoopSkillState instance per loop attempt. No behaviour change: logic was moved verbatim into the class's hydrate/applySuccessfulResult/ markFormInputSubmitted methods. Regenerated docs/api-reference/veryfront/agent.md — only a line-number pin shift for AgentRuntime's source link.
Point future skill-policy work at AgentLoopSkillState as the single owner, so the seam introduced in the prior two commits gets used instead of re-duplicated.
The comment incorrectly implied the two agent loops differ in how they determine the submitted predicate. In reality, both use the same helpers but with different scopes: callers use a broader predicate (isSubmittedFormInputExecutionResult) than the method applies internally (isSubmittedFormInputResult). They can disagree, so the flag cannot be safely recomputed inside the method without changing behavior.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdded ChangesAgent loop skill state
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AgentLoop
participant AgentLoopSkillState
participant ToolExecution
AgentLoop->>AgentLoopSkillState: hydrate message history and runtime context
AgentLoop->>AgentLoopSkillState: read active policy and tool state
ToolExecution->>AgentLoopSkillState: apply successful result
AgentLoopSkillState-->>AgentLoop: return updated policy and submission state
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (2)
src/agent/runtime/agent-loop-skill-state.ts (2)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse aliases for cross-module imports.
Both files import
src/agent/types.tsthrough a parent-relative path. Use the internal alias for this cross-module import.
src/agent/runtime/agent-loop-skill-state.ts#L1-L1: replace../types.tswith#veryfront/agent/types.ts.src/agent/runtime/agent-loop-skill-state.test.ts#L5-L5: replace../types.tswith#veryfront/agent/types.ts.As per coding guidelines, use
#veryfront/*for internal source imports. Based on learnings, use relative imports only for same-directory sibling files.🤖 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/agent/runtime/agent-loop-skill-state.ts` at line 1, Replace the parent-relative Message imports in src/agent/runtime/agent-loop-skill-state.ts (line 1) and src/agent/runtime/agent-loop-skill-state.test.ts (line 5) with the `#veryfront/agent/types.ts` internal alias, keeping relative imports only for same-directory siblings.Sources: Coding guidelines, Learnings
16-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse direct public documentation.
Both changed documentation blocks use an em dash and indirect phrasing. Use short sentences and ASCII punctuation.
src/agent/runtime/agent-loop-skill-state.ts#L16-L21: state the caller construction and isolation requirements in direct sentences.CONTEXT.md#L20-L29: state the loop ownership and mutation lifecycle in direct sentences.As per coding guidelines, public TypeScript and Markdown copy must use direct, concise wording and ASCII punctuation.
🤖 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/agent/runtime/agent-loop-skill-state.ts` around lines 16 - 21, Rewrite the documentation in src/agent/runtime/agent-loop-skill-state.ts lines 16-21 using short, direct sentences with ASCII punctuation; state that callers construct one state via hydrate per attempt and never share it across concurrent runs. Rewrite the related documentation in CONTEXT.md lines 20-29 using direct sentences with ASCII punctuation; state that each loop owns its state and mutates it in place through the tool-result lifecycle.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.
Inline comments:
In `@src/agent/runtime/index.ts`:
- Around line 1565-1572: Update the generatedToolResult handling branch before
its early return to apply the same successful-result transitions used by the
main path, including skillState.applySuccessfulResult and
skillState.markFormInputSubmitted with isSubmittedFormInputExecutionResult. Add
focused generate-loop coverage confirming generated load_skill activates its
policy and generated form_input sets hasSubmittedFormInput before the next
iteration.
---
Nitpick comments:
In `@src/agent/runtime/agent-loop-skill-state.ts`:
- Line 1: Replace the parent-relative Message imports in
src/agent/runtime/agent-loop-skill-state.ts (line 1) and
src/agent/runtime/agent-loop-skill-state.test.ts (line 5) with the
`#veryfront/agent/types.ts` internal alias, keeping relative imports only for
same-directory siblings.
- Around line 16-21: Rewrite the documentation in
src/agent/runtime/agent-loop-skill-state.ts lines 16-21 using short, direct
sentences with ASCII punctuation; state that callers construct one state via
hydrate per attempt and never share it across concurrent runs. Rewrite the
related documentation in CONTEXT.md lines 20-29 using direct sentences with
ASCII punctuation; state that each loop owns its state and mutates it in place
through the tool-result lifecycle.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7ec637d9-acc1-44f4-8cba-1f7dd893f7db
📒 Files selected for processing (5)
CONTEXT.mddocs/api-reference/veryfront/agent.mdsrc/agent/runtime/agent-loop-skill-state.test.tssrc/agent/runtime/agent-loop-skill-state.tssrc/agent/runtime/index.ts
The generate loop's generatedToolResult branch returned before the skill-state transitions ran, so a successful generated load_skill result did not activate its policy and a generated form_input result did not set hasSubmittedFormInput. The streaming loop already applies both transitions in its matchingResult and persistedResult paths; this restores the same handling in the generate loop. Also drop the em dash from the AgentLoopSkillState doc block and the CONTEXT.md entry, and state both in direct sentences.
|
Addressed the two nitpicks from the review body in 8e27194. Direct public documentation — applied. The Aliases for cross-module imports — not applied. Every file in |
|
@kentaro-wakayama heads up — I think our messages crossed. You pushed 8e27194 applying the CodeRabbit fix at 04:53; I posted an investigation reply on that thread at 05:01 that reaches the opposite conclusion. Flagging it rather than touching your commit, since this is your call. The short version: the branch is unreachable for Three links in the chain:
Confirmed by driving the real loop, not by reading. With real provider behaviour (model emits only a That is also why the commit has no test: any test that fails without it must fabricate that response, and would lock in provider behaviour that cannot occur. Your call on what to do, and either is defensible:
One more thing the trace turned up, unrelated and untouched: Full evidence is in the resolved thread on |
Defer to the reachability trace on #3440: this branch is the generate-path analogue of the streaming providerExecuted === true branch, which also omits the transitions. Both handle only provider-executed results, so the omission is deliberate rather than an oversight. The doc-style fix from 8e27194 stays.
|
Reverted my change in 54da8f5. Your read is better than mine. We landed on the same reachability facts independently: The part I got wrong is which branch this one mirrors. I paired it with the streaming loop
Worth filing the |
|
Thanks — and your correction on the pairing is the better insight, so I've built the comment around it rather than around my original framing. You're right that this branch mirrors the streaming loop's Confirmed On the Your reasoning was right and the consequence goes one step further:
Two lines to fix. The reason it survived is worth noting: the neighbouring One adjacent thing flagged but not swept into #3446: |
Add explanatory comment above the generatedToolResult branch in executeAgentLoop (non-streaming path) to document why skill-state transitions are intentionally absent. The branch handles provider-executed tools only (web_search/web_fetch) and mirrors the streaming loop's providerExecuted===true path, not the locally-executed paths. This clarifies the distinction for reviewers and prevents re-raising the resolved concern.
2 participants
Gives the request-scoped active-skill policy one owner, so a skill-policy fix stops having to be hand-applied to two agent loops. Zero behaviour change.
The defect
src/agent/runtime/index.tscontains two agent loops —executeAgentLoopandexecuteAgentLoopStreaming. Both opened with a byte-identical 18-line skill-policy block:hydrateActiveSkillStateFromMessages, fouractiveSkill*letbindings, and the entireapplySuccessfulSkillResultclosure. A second duplicated transition (removeFormInputAfterSubmission+ ahasSubmittedFormInputflag) appeared at 4 sites.So a skill-policy bug fix had to be applied twice, with nothing enforcing the copies stayed in sync. That is the defect this PR removes.
The change
New
src/agent/runtime/agent-loop-skill-state.ts— the single owner of the request-scoped active-skill policy for one agent loop attempt: which skill is active, what it permits, and how that changes when a skill activates or a form input is submitted.Both loops now construct one
AgentLoopSkillStateand call methods on it.index.tsis net −46 lines.CONTEXT.mdrecords the domain term.The state is a class with mutable fields, deliberately: it is read ~23 times in one loop and ~26 in the other and is mutated in place, so an immutable snapshot would have changed semantics. There is no module-level or static mutable state — the original code's "Request-scoped skill policy (not class-level mutable state)" guarantee is preserved, and the tests prove two instances are independent behaviourally rather than asserting it in prose.
What this PR deliberately does NOT do
An earlier plan proposed unifying the two loops into one shared step function. I measured them and rejected that:
executeAgentLoopsupportstoolReplacements, forking ~40 lines of config resolution with no streaming counterpart. Streaming carriescontroller/encoder/callbacks/textPartIdplumbing with no non-streaming counterpart. Non-streaming wraps inwithSpan("agent.execution_loop"); streaming does not. The parameter orders even differ —abortSignalandtemperatureModelStringare swapped.Unifying those is a redesign reconciling two feature sets, not a refactor — and the riskiest logic (skill-policy edge cases) sits in the shared part. The duplication was the defect; the loops being separate was not.
Evidence
deno task test:unit: baseline 3801 passed / 27907 steps / 0 failed → 3802 / 27922 / 0 failed. Delta is exactly +1 file / +15 steps, matching the new test file 1:1 — zero regressions across the 66 co-located test files insrc/agent/runtime/.deno task verify:quickexit 0.deno check src/agent/index.tsclean.docs/api-reference/veryfront/agent.mdis a single line-pin shift, regenerated with Deno 2.7.7 matching CI's pin.Reviewer note: why
markFormInputSubmittedtakes a booleanIt takes a precomputed
submittedflag rather than recomputing it, because the caller's predicate is deliberately broader than the one applied internally, and the two genuinely disagree:isSubmittedFormInputExecutionResult— recurses into nested objects to depth 3, ignores error markers.removeFormInputAfterSubmissioninternally appliesisSubmittedFormInputResult— shallow, only unwrapsresponse/output, bails on an error marker.They differ on inputs like
{ data: { submitted: true } }. So the flag cannot be recomputed inside the method without changing behaviour. The doc comment says so explicitly, to stop a future caller from "simplifying" it.Known follow-ups (not blocking)
AgentLoopSkillState's fields are public and mutable, so the single-owner guarantee is convention-only today. No caller bypasses the transitions (verified). Private fields + getters would be the hardening if a third consumer appears.hasSubmittedFormInputResult(messages) || runtimeContext?.[SUBMITTED_FORM_INPUT_CONTEXT_KEY] === truenow lives in two places (hydrateandfilterToolsAfterSubmittedFormInput). This PR reduced it from three copies to two; a one-line exported helper would finish it.readApiErrorMessage(duplicated across two runtime clients) andisWithinJsonSizeLimit(4 copies) remain — separate PR.Summary by CodeRabbit
New Features
Documentation
Tests