Add Gerrit AI fertilizer planning agent and fdm-agents package#534
Merged
SvenVw merged 92 commits intoApr 1, 2026
Conversation
…er application plan
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
.github/workflows/tests.yml (1)
359-374: Remove PostgreSQL service container from fdm-agents test step.The
fdm-agentstests don't reference PostgreSQL or any database connection. None of the five test files (agent.test.ts,default.test.ts,one-shot.test.ts,fertilizer-planner/index.test.ts,index.test.ts) use thePOSTGRES_*environment variables, andfdm-agentshas no database package dependencies. While fdm-agents depends on fdm-core for types and utilities, the test execution doesn't invoke any database operations.The PostgreSQL service container and associated environment variables (lines 359-374 and env vars in the test step) can be removed to eliminate the ~30s startup overhead from this CI job.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/tests.yml around lines 359 - 374, Remove the PostgreSQL service and its env vars from the fdm-agents test job: delete the services.postgres block (including image, env: POSTGRES_USER and POSTGRES_PASSWORD, and the options health-check settings) from the fdm-agents test step so the CI no longer starts the postgres container; also remove any POSTGRES_* env entries referenced in the same test step so no unused DB vars are set.fdm-app/app/components/blocks/gerrit/loading.tsx (1)
39-47: Consider adding live-region semantics for loading state.This improves screen-reader feedback while Gerrit is generating a plan.
♿ Suggested accessibility tweak
- <CardContent> - <div className="flex items-center gap-3 text-sm text-muted-foreground"> - <Spinner className="h-4 w-4 shrink-0 text-primary" /> + <CardContent> + <div + role="status" + aria-live="polite" + aria-busy="true" + className="flex items-center gap-3 text-sm text-muted-foreground" + > + <Spinner + aria-hidden="true" + className="h-4 w-4 shrink-0 text-primary" + /> <span> Dit kan enkele minuten tot een kwartier duren, afhankelijk van het gekozen model en de omvang van het bedrijf. </span> </div> </CardContent>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/gerrit/loading.tsx` around lines 39 - 47, The loading block lacks live-region semantics for screen readers; update the container (the div inside CardContent that wraps Spinner and the message) to include aria-live="polite" (or role="status") and aria-busy="true", and add a visually-hidden text node (e.g., a span with sr-only) that announces "Generating plan, please wait" so assistive tech gets immediate feedback while Gerrit is generating a plan; keep Spinner and visible message unchanged.fdm-agents/src/agents/gerrit/agent.ts (1)
12-22: Add upfront validation for Gemini API credentials.The
apiKeyparameter is optional, and while@google/adkfalls back to theGEMINI_API_KEYenvironment variable, misconfiguration (missing both) defers the error to runtime execution rather than failing fast. A guard at agent creation improves debugging.🛠️ Suggested guard
export function createFertilizerPlannerAgent( fdm: FdmType, apiKey?: string, model?: string, ) { + if (!apiKey && !process.env.GEMINI_API_KEY) { + throw new Error( + "Gemini API key is required (pass apiKey or set GEMINI_API_KEY).", + ) + } + return new LlmAgent({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-agents/src/agents/gerrit/agent.ts` around lines 12 - 22, Add an upfront guard in createFertilizerPlannerAgent to validate Gemini API credentials: check that either the passed apiKey is present or process.env.GEMINI_API_KEY exists before calling createDefaultModel and constructing the LlmAgent; if neither is available, throw a clear error (or return a rejected result) with a message indicating the missing Gemini API key so the failure occurs at agent creation rather than at runtime inside LLM calls.fdm-app/app/routes/farm.$b_id_farm.$calendar.gerrit.tsx (2)
1122-1124: RedundantisGeneratingvariable.The variable
isGenerating(line 1122-1124) duplicates the logic ofisAIGenerating(line 904-906). Consider usingisAIGeneratingconsistently throughout the component.♻️ Use isAIGenerating consistently
- const isGenerating = - navigation.state === "submitting" && - navigation.formData?.get("intent") === "generate" - return ( <> ... - {isGenerating ? ( + {isAIGenerating ? ( <GerritLoading />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.gerrit.tsx around lines 1122 - 1124, Remove the redundant isGenerating variable and replace its usages with the existing isAIGenerating to avoid duplicated logic; locate the declaration of isGenerating and any references to it in this component (e.g., places checking navigation.state and navigation.formData.intent) and switch them to use isAIGenerating (declared earlier at lines around the isAIGenerating definition) so the component consistently relies on the single source of truth.
178-185: Year validation fallback may mask misconfiguration.The year defaults to
"2025"if not in["2025", "2026"], which could hide issues where an unsupported year reaches this function. Since the UI already blocks unsupported years (line 880-881), this silent fallback might never trigger, but it could cause confusing behavior if it does.💡 Consider logging or throwing for unsupported years
const year = (["2025", "2026"].includes(calendar) ? calendar : "2025") as | "2025" | "2026" + if (!["2025", "2026"].includes(calendar)) { + console.warn(`[computePlanMetrics] Unsupported calendar year ${calendar}, defaulting to 2025`) + } const normFuncs = createFunctionsForNorms("NL", year)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.gerrit.tsx around lines 178 - 185, The code silently falls back to "2025" for any calendar value not in ["2025","2026"], which can mask misconfiguration; change the logic around calendar/year so that you validate the incoming calendar value and either throw an error or log a clear warning before calling createFunctionsForNorms and createFunctionsForFertilizerApplicationFilling; specifically, inspect the variable calendar, if it is not "2025" or "2026" then throw or processLogger.warn with the unsupported value and return a 400/appropriate response, otherwise set year to the validated value and pass it to createFunctionsForNorms and createFunctionsForFertilizerApplicationFilling.fdm-agents/src/tools/fertilizer-planner/index.ts (2)
176-184: Emptyb_lu_cataloguemay cause unreliable advice.When
mainLuis undefined (no cultivation found on May 15), passing an empty string togetNutrientAdvicecould produce invalid or generic advice. Consider either skipping the advice fetch for fields without a main cultivation or returning a clear indicator that advice is unavailable.♻️ Suggested approach
+ if (!mainLu?.b_lu_catalogue) { + return { b_id, advice: null, reason: "No main cultivation found" } + } const advice = await getNutrientAdvice(fdm, { - b_lu_catalogue: mainLu?.b_lu_catalogue || "", + b_lu_catalogue: mainLu.b_lu_catalogue, b_centroid: field.b_centroid ?? [0, 0], currentSoilData: currentSoilData, nmiApiKey: nmiApiKey || "", b_bufferstrip: field.b_bufferstrip, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-agents/src/tools/fertilizer-planner/index.ts` around lines 176 - 184, The code passes an empty string for b_lu_catalogue to getNutrientAdvice when mainLu is undefined, which can produce unreliable advice; update the logic around getNutrientAdvice (referencing mainLu and getNutrientAdvice) to detect when mainLu is missing and either skip calling getNutrientAdvice and return an explicit marker (e.g., advice: null or adviceUnavailable: true) for that b_id, or pass a clear "unknown" indicator to the advice consumer so callers know advice is not available for that field instead of passing a blank catalogue string.
692-694: Buffer strip violation detection may miss some cases.The check
!r.isValid && r.erroridentifies buffer strip violations, but any field that returned early with an error (line 437-444) will match this condition. This is currently correct since buffer strips are the only early-return error case, but it's fragile if other error cases are added later.♻️ Consider a more explicit flag
return { b_id: fieldData.b_id, b_area: fieldInfo.b_area, error: "Field is a buffer strip and cannot receive fertilizer applications.", isValid: false, + isBufferStripViolation: true, fieldMetrics: null, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-agents/src/tools/fertilizer-planner/index.ts` around lines 692 - 694, The current buffer-strip detection uses hasBufferStripViolations = fieldResults.some((r) => !r.isValid && r.error) which can misclassify other early-return errors; update the code to use an explicit flag (e.g. add r.isBufferStripViolation or r.errorCode === 'BUFFER_STRIP_VIOLATION') on the result objects where the buffer-strip early return is created (the function that builds/returns those fieldResults) and change the check to look for that explicit flag (hasBufferStripViolations = fieldResults.some(r => r.isBufferStripViolation || r.errorCode === 'BUFFER_STRIP_VIOLATION')) so the intent is clear and future error types won't be mistaken for buffer-strip violations.fdm-agents/src/index.ts (1)
131-132: Misleading variable namesafeContext.The variable
safeContextat line 132 is assigned the raw (unsanitized)additionalContext, which is misleading since it's not yet "safe". The actual sanitization happens insidebuildFertilizerPlanPrompt. Consider renaming for clarity.♻️ Rename for clarity
const validatedStrategies = FertilizerPlanStrategiesSchema.parse(strategies) - const safeContext = additionalContext ?? "None" + const rawContext = additionalContext ?? "None" const agent = createFertilizerPlannerAgent(fdm, geminiApiKey) const input = buildFertilizerPlanPrompt( farmData, validatedStrategies, calendar, - safeContext, + rawContext, fieldsSummary, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-agents/src/index.ts` around lines 131 - 132, The variable name safeContext is misleading because it holds the raw additionalContext (sanitization occurs later in buildFertilizerPlanPrompt); rename safeContext to a clearer name like rawContext or providedContext where it’s assigned (currently const safeContext = additionalContext ?? "None") and update all subsequent references in this file (including the call to buildFertilizerPlanPrompt and any logging) to use the new name so it correctly reflects that sanitization is not performed here; keep FertilizerPlanStrategiesSchema.parse(strategies) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/tests.yml:
- Around line 407-417: Add a build step for the fdm-calculator package before
running tests: create a new workflow step named something like "Build
fdm-calculator" that runs "pnpm build" with working-directory set to
./fdm-calculator and place it before the existing "Run tests with coverage"
step; this ensures the `@nmi-agro/fdm-calculator` module (imported from
src/types.d.ts and src/tools/fertilizer-planner/index.ts) is compiled and
available for the fdm-agents test run.
In `@fdm-agents/src/tools/fertilizer-planner/index.ts`:
- Around line 866-870: The loop over fieldResults incorrectly checks
result.nBalance; change the guard to reference result.fieldMetrics.nBalance (use
optional chaining: result.fieldMetrics?.nBalance) and update subsequent accesses
so nh3FromFertilizers reads from
result.fieldMetrics.nBalance.emission?.ammonia?.fertilizers?.total instead of
result.nBalance; ensure the null-coalescing behavior (?? 0) remains the same so
NH3 calculations run when fieldMetrics.nBalance exists.
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.gerrit.tsx:
- Around line 630-636: The agentContext object passed to runOneShotAgent is
missing the strategies and additionalContext keys expected by the agent tools;
update the agentContext (the object currently constructed with principalId,
b_id_farm, calendar, nmiApiKey) to include strategies (default to an appropriate
array or the strategies value used in fdm-agents, e.g., [] or the app's strategy
list) and additionalContext (an object for any extra metadata, e.g., {} or the
same shape used in fdm-agents/src/index.ts), then pass this augmented
agentContext into runOneShotAgent so the agent receives the same keys as shown
in fdm-agents (look for runOneShotAgent and agentContext in this file to locate
the change).
- Around line 857-860: The redirect call using redirectWithSuccess currently
builds a path without a leading slash
("farm/${b_id_farm}/${calendar}/rotation"), which is inconsistent with other
absolute redirects and can be treated as relative by Remix; update the string
passed to redirectWithSuccess to include a leading slash
("/farm/${b_id_farm}/${calendar}/rotation") where the redirectWithSuccess(...)
invocation appears so it uses an absolute root path consistent with other calls
like "/farm/${b_id_farm}" and "/farm/${b_id_farm}/fertilizers".
In `@fdm-core/src/fertilizer.d.ts`:
- Line 34: The type annotation for the property p_k_rt contains a typo "nulll"
which is invalid TypeScript; update the declaration of p_k_rt to use the correct
nullable type (replace "nulll" with "null") so it reads "p_k_rt: number | null"
in the fertilizer declaration to restore proper type-checking.
---
Nitpick comments:
In @.github/workflows/tests.yml:
- Around line 359-374: Remove the PostgreSQL service and its env vars from the
fdm-agents test job: delete the services.postgres block (including image, env:
POSTGRES_USER and POSTGRES_PASSWORD, and the options health-check settings) from
the fdm-agents test step so the CI no longer starts the postgres container; also
remove any POSTGRES_* env entries referenced in the same test step so no unused
DB vars are set.
In `@fdm-agents/src/agents/gerrit/agent.ts`:
- Around line 12-22: Add an upfront guard in createFertilizerPlannerAgent to
validate Gemini API credentials: check that either the passed apiKey is present
or process.env.GEMINI_API_KEY exists before calling createDefaultModel and
constructing the LlmAgent; if neither is available, throw a clear error (or
return a rejected result) with a message indicating the missing Gemini API key
so the failure occurs at agent creation rather than at runtime inside LLM calls.
In `@fdm-agents/src/index.ts`:
- Around line 131-132: The variable name safeContext is misleading because it
holds the raw additionalContext (sanitization occurs later in
buildFertilizerPlanPrompt); rename safeContext to a clearer name like rawContext
or providedContext where it’s assigned (currently const safeContext =
additionalContext ?? "None") and update all subsequent references in this file
(including the call to buildFertilizerPlanPrompt and any logging) to use the new
name so it correctly reflects that sanitization is not performed here; keep
FertilizerPlanStrategiesSchema.parse(strategies) unchanged.
In `@fdm-agents/src/tools/fertilizer-planner/index.ts`:
- Around line 176-184: The code passes an empty string for b_lu_catalogue to
getNutrientAdvice when mainLu is undefined, which can produce unreliable advice;
update the logic around getNutrientAdvice (referencing mainLu and
getNutrientAdvice) to detect when mainLu is missing and either skip calling
getNutrientAdvice and return an explicit marker (e.g., advice: null or
adviceUnavailable: true) for that b_id, or pass a clear "unknown" indicator to
the advice consumer so callers know advice is not available for that field
instead of passing a blank catalogue string.
- Around line 692-694: The current buffer-strip detection uses
hasBufferStripViolations = fieldResults.some((r) => !r.isValid && r.error) which
can misclassify other early-return errors; update the code to use an explicit
flag (e.g. add r.isBufferStripViolation or r.errorCode ===
'BUFFER_STRIP_VIOLATION') on the result objects where the buffer-strip early
return is created (the function that builds/returns those fieldResults) and
change the check to look for that explicit flag (hasBufferStripViolations =
fieldResults.some(r => r.isBufferStripViolation || r.errorCode ===
'BUFFER_STRIP_VIOLATION')) so the intent is clear and future error types won't
be mistaken for buffer-strip violations.
In `@fdm-app/app/components/blocks/gerrit/loading.tsx`:
- Around line 39-47: The loading block lacks live-region semantics for screen
readers; update the container (the div inside CardContent that wraps Spinner and
the message) to include aria-live="polite" (or role="status") and
aria-busy="true", and add a visually-hidden text node (e.g., a span with
sr-only) that announces "Generating plan, please wait" so assistive tech gets
immediate feedback while Gerrit is generating a plan; keep Spinner and visible
message unchanged.
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.gerrit.tsx:
- Around line 1122-1124: Remove the redundant isGenerating variable and replace
its usages with the existing isAIGenerating to avoid duplicated logic; locate
the declaration of isGenerating and any references to it in this component
(e.g., places checking navigation.state and navigation.formData.intent) and
switch them to use isAIGenerating (declared earlier at lines around the
isAIGenerating definition) so the component consistently relies on the single
source of truth.
- Around line 178-185: The code silently falls back to "2025" for any calendar
value not in ["2025","2026"], which can mask misconfiguration; change the logic
around calendar/year so that you validate the incoming calendar value and either
throw an error or log a clear warning before calling createFunctionsForNorms and
createFunctionsForFertilizerApplicationFilling; specifically, inspect the
variable calendar, if it is not "2025" or "2026" then throw or
processLogger.warn with the unsupported value and return a 400/appropriate
response, otherwise set year to the validated value and pass it to
createFunctionsForNorms and createFunctionsForFertilizerApplicationFilling.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9856cb94-135c-4320-a722-5acfb2876ac3
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (22)
.github/workflows/tests.ymlREADME.mdfdm-agents/README.mdfdm-agents/package.jsonfdm-agents/src/agents/gerrit/agent.tsfdm-agents/src/index.tsfdm-agents/src/models/default.test.tsfdm-agents/src/tools/fertilizer-planner/index.tsfdm-app/.env.examplefdm-app/app/components/blocks/gerrit/feedback.tsxfdm-app/app/components/blocks/gerrit/loading.tsxfdm-app/app/components/blocks/gerrit/norm-bar.tsxfdm-app/app/components/blocks/gerrit/plan-table.tsxfdm-app/app/components/blocks/gerrit/schema.tsfdm-app/app/components/blocks/gerrit/strategy-form.tsxfdm-app/app/components/blocks/gerrit/summary-cards.tsxfdm-app/app/lib/config.server.tsfdm-app/app/lib/error.tsfdm-app/app/routes/farm.$b_id_farm.$calendar.gerrit.tsxfdm-core/src/fertilizer.d.tsfdm-docs/docs/fdm-agents/index.mdpnpm-workspace.yaml
✅ Files skipped from review due to trivial changes (3)
- fdm-app/.env.example
- fdm-agents/README.md
- fdm-agents/package.json
🚧 Files skipped from review as they are similar to previous changes (6)
- pnpm-workspace.yaml
- fdm-agents/src/models/default.test.ts
- fdm-app/app/components/blocks/gerrit/norm-bar.tsx
- fdm-app/app/components/blocks/gerrit/summary-cards.tsx
- fdm-app/app/components/blocks/gerrit/schema.ts
- fdm-app/app/components/blocks/gerrit/plan-table.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fdm-agents/src/agents/gerrit/agent.ts`:
- Around line 99-102: The JSON example under normsFilling is not valid because
it uses "applicationFilling": [...]; update the prompt/template so the contract
is parseable by replacing the placeholder with an actual empty array
("applicationFilling": []) or move the explanatory placeholder text outside the
JSON block; locate the JSON example associated with normsFilling (look for the
normsFilling constant or the prompt/template string in agent.ts where
applicationFilling appears) and make this single change so downstream JSON.parse
calls won’t break.
In `@fdm-agents/src/index.ts`:
- Around line 52-61: sanitizeAdditionalContext currently leaves the exact prompt
boundary markers intact; update sanitizeAdditionalContext to strip the literal
delimiters --- BEGIN/END ADDITIONAL USER CONTEXT --- before final trimming so a
user cannot inject a closing marker. Specifically, after the existing noTags
step in sanitizeAdditionalContext, add a replacement that matches
/---\s*(BEGIN|END)\s+ADDITIONAL USER CONTEXT\s*---/gim (or equivalent) and
replace with a safe token like "[removed]", then run the existing fallback regex
(adjusted to operate on this new intermediate string) and finally trim and slice
as before.
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.gerrit.tsx:
- Around line 522-534: The action handler (export async function action) lacks
the same route guards as the React component: before processing
formData/intents, retrieve timeframe and calendar via getTimeframe(params) and
getCalendar(params) and validate the calendar year is supported (block years
outside 2025/2026) and that the Gerrit rollout flag/condition used by the
component is enabled; if validation fails, immediately return a 400/403 response
(e.g., throw data("Unsupported year" or "Gerrit not enabled", { status: 400/403
})) to reject generate/accept requests; ensure this check runs before cloning
request, reading formData, or calling computePlanMetrics so malformed years
cannot slip through.
- Around line 839-859: The loop over plan.plan currently trusts client-supplied
field.b_id and may write to fields outside the current farm; before calling
addFertilizerApplication, verify that field.b_id exists in rawFields (or the
farm-scoped set) for the current b_id_farm and session.principal_id—if not,
throw or skip with an explicit error; perform this check inside the same loop
that processes each field (reference plan.plan, field.b_id, rawFields, and
addFertilizerApplication) so only validated field IDs are written.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0db37a85-2b09-40c3-8083-5fd8502323f4
📒 Files selected for processing (6)
.github/workflows/tests.ymlfdm-agents/src/agents/gerrit/agent.tsfdm-agents/src/index.tsfdm-agents/src/tools/fertilizer-planner/index.tsfdm-app/app/components/blocks/gerrit/loading.tsxfdm-app/app/routes/farm.$b_id_farm.$calendar.gerrit.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- fdm-app/app/components/blocks/gerrit/loading.tsx
- fdm-agents/src/tools/fertilizer-planner/index.ts
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
fdm-app/app/routes/farm.$b_id_farm.$calendar.gerrit.tsx (2)
498-503:⚠️ Potential issue | 🟠 MajorReject unsupported calendars before the action does any work.
The component blocks years outside 2025/2026, but a direct POST still reaches
generate/accept. That is especially risky here becausecomputePlanMetrics()later falls back its calculator year to"2025"while the submitted timeframe is left unchanged. If there is a server-side rollout check for Gerrit, this is the right place to mirror that too.🛡️ Server-side year guard
const timeframe = getTimeframe(params) const calendar = getCalendar(params) if (!b_id_farm) throw data("Missing farm ID", { status: 400 }) + if (!["2025", "2026"].includes(calendar)) { + throw data("Unsupported calendar", { status: 400 }) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.gerrit.tsx around lines 498 - 503, Reject unsupported calendar/timeframes at the start of the action: after obtaining params and calling getTimeframe(params) and getCalendar(params) in the action function, validate that the calendar/year falls within the supported set (e.g., 2025/2026) and immediately return an appropriate error response (or throw) if not supported, before any work such as generate, accept, or computePlanMetrics() runs; mirror the same server-side rollout guard used by the UI to prevent falling back to a different calculator year and ensure generate/accept handlers never receive unsupported timeframe values.
214-255:⚠️ Potential issue | 🟠 MajorDon't publish farm totals from a partial metric pass.
When a field fails norm/filling calculation,
Promise.allSettled()drops it, but the remaining fields are still aggregated into farm-level totals. Those totals then look authoritative even though one or more fields were omitted. If any field metric fails, returnnull/incompletetotals instead of partial compliance numbers.💡 Minimal guard
- if (validFields.length === 0) return null + if ( + fieldResults.some((result) => result.status === "rejected") || + validFields.length === 0 + ) { + return null + }Also applies to: 440-495
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.gerrit.tsx around lines 214 - 255, The farm totals are being calculated after using Promise.allSettled on enrichedPlan fields (via the fieldResults from Promise.allSettled that wraps normFuncs.collectInputForNorms and calculateNormForManure/Phosphate/Nitrogen), so when any field promise rejects you must not aggregate partial results; instead detect any fieldResults entry with status === 'rejected' (or where the value is missing/invalid) and set the farm-level totals to null/incomplete (or return early) rather than summing the fulfilled entries. Apply the same guard wherever you aggregate field-level metrics into farm totals (including the other aggregation block around lines 440-495) so that any rejected field causes the farm totals to be marked incomplete.
🧹 Nitpick comments (2)
fdm-agents/src/runners/one-shot.test.ts (2)
20-209: Add a direct timeout regression test.
runOneShotAgent()now has timeout behavior, but this suite never drives a non-terminating stream with a smalltimeoutMs. A dedicated case would protect theAgentTimeoutErrorpath and any future timeout-cleanup fix.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-agents/src/runners/one-shot.test.ts` around lines 20 - 209, Add a new test that drives the timeout path by having mockRunEphemeral return a non-terminating async generator (e.g., an async function* that yields nothing or awaits forever) and calling runOneShotAgent(mockAgent, "Generate plan", undefined, undefined, { timeoutMs: 10 }) (or whichever param name supports timeout) and assert the promise rejects with AgentTimeoutError; reference mockRunEphemeral to inject the stream and the AgentTimeoutError symbol for the expected throw, and keep the test fast by using a very small timeoutMs so CI won't hang.
4-16: Prefer a manual seam overvi.mock("@google/adk").This suite now depends on module-level Vitest mocks for all ADK behavior. In this repo those mocks have been a recurring source of brittle tests; a small injectable runner/helper seam would let you test with plain objects and real async generators instead.
Based on learnings: When writing unit tests for the fdm project, avoid using Vitest's mocking utilities (vi) as it has caused problems in the past.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-agents/src/runners/one-shot.test.ts` around lines 4 - 16, The test currently uses a module-level Vitest mock for "@google/adk" (mockRunEphemeral, InMemoryRunner, isFinalResponse, stringifyContent), which is brittle; replace this with a manual seam by removing vi.mock and constructing a plain fake InMemoryRunner object/class in the test that implements runEphemeral (as a real async generator or async function) plus stubbed isFinalResponse and stringifyContent functions, then pass that fake into the code under test via dependency injection or a test-only factory/constructor parameter (or import the module and call a setter to replace the runner) so the test uses plain objects and real async behavior instead of vi.mock.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fdm-agents/src/index.test.ts`:
- Around line 9-19: Remove the file-scoped vi.mock calls and replace them with
per-test manual mocks: delete the vi.mock("./agents/gerrit/agent", ...) and
vi.mock("./runners/one-shot", ...) calls, and in each test create and inject
local mock functions for createFertilizerPlannerAgent and runOneShotAgent (so
each test gets a fresh mock and no shared call history); ensure tests assert
against the locally created runOneShotAgent mock (and reset or recreate it
between tests) rather than relying on toHaveBeenLastCalledWith from a global
mock.
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.gerrit.tsx:
- Around line 727-756: PostHog calls can throw and must not break the action:
wrap the call to PostHogClient() result usage (posthog.capture({...}) and await
posthog.flush()) in a local try/catch block (around the code that builds and
calls posthog.capture and flush) so any errors are caught and swallowed or
logged but not re-thrown; reference PostHogClient, posthog.capture,
posthog.flush, and the surrounding properties like session.principal_id,
modelName, rawResult, toolCalls, b_id_farm and calendar when locating where to
add the try/catch. Ensure the catch logs the error (do not change action return
flow) and continues execution.
- Around line 121-123: The redirect call in the route handler uses a relative
path redirect("./farm") which resolves to a non-existent nested route; update
the redirect to use the absolute path "/farm" instead so that when farm is falsy
the code in this module (the handler using redirect("./farm")) sends users to
the global farms overview; locate the redirect("./farm") invocation in this file
(the route module handling /farm/:b_id_farm/:calendar/gerrit) and replace the
relative path with "/farm".
---
Duplicate comments:
In `@fdm-app/app/routes/farm`.$b_id_farm.$calendar.gerrit.tsx:
- Around line 498-503: Reject unsupported calendar/timeframes at the start of
the action: after obtaining params and calling getTimeframe(params) and
getCalendar(params) in the action function, validate that the calendar/year
falls within the supported set (e.g., 2025/2026) and immediately return an
appropriate error response (or throw) if not supported, before any work such as
generate, accept, or computePlanMetrics() runs; mirror the same server-side
rollout guard used by the UI to prevent falling back to a different calculator
year and ensure generate/accept handlers never receive unsupported timeframe
values.
- Around line 214-255: The farm totals are being calculated after using
Promise.allSettled on enrichedPlan fields (via the fieldResults from
Promise.allSettled that wraps normFuncs.collectInputForNorms and
calculateNormForManure/Phosphate/Nitrogen), so when any field promise rejects
you must not aggregate partial results; instead detect any fieldResults entry
with status === 'rejected' (or where the value is missing/invalid) and set the
farm-level totals to null/incomplete (or return early) rather than summing the
fulfilled entries. Apply the same guard wherever you aggregate field-level
metrics into farm totals (including the other aggregation block around lines
440-495) so that any rejected field causes the farm totals to be marked
incomplete.
---
Nitpick comments:
In `@fdm-agents/src/runners/one-shot.test.ts`:
- Around line 20-209: Add a new test that drives the timeout path by having
mockRunEphemeral return a non-terminating async generator (e.g., an async
function* that yields nothing or awaits forever) and calling
runOneShotAgent(mockAgent, "Generate plan", undefined, undefined, { timeoutMs:
10 }) (or whichever param name supports timeout) and assert the promise rejects
with AgentTimeoutError; reference mockRunEphemeral to inject the stream and the
AgentTimeoutError symbol for the expected throw, and keep the test fast by using
a very small timeoutMs so CI won't hang.
- Around line 4-16: The test currently uses a module-level Vitest mock for
"@google/adk" (mockRunEphemeral, InMemoryRunner, isFinalResponse,
stringifyContent), which is brittle; replace this with a manual seam by removing
vi.mock and constructing a plain fake InMemoryRunner object/class in the test
that implements runEphemeral (as a real async generator or async function) plus
stubbed isFinalResponse and stringifyContent functions, then pass that fake into
the code under test via dependency injection or a test-only factory/constructor
parameter (or import the module and call a setter to replace the runner) so the
test uses plain objects and real async behavior instead of vi.mock.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4338543a-417d-433c-a7a4-64bf6f6cf7a8
📒 Files selected for processing (10)
fdm-agents/src/agents/gerrit/agent.test.tsfdm-agents/src/agents/gerrit/agent.tsfdm-agents/src/index.test.tsfdm-agents/src/index.tsfdm-agents/src/runners/one-shot.test.tsfdm-agents/src/runners/one-shot.tsfdm-agents/src/tools/fertilizer-planner/index.test.tsfdm-app/app/components/blocks/gerrit/types.tsfdm-app/app/routes/farm.$b_id_farm.$calendar.gerrit.tsxfdm-calculator/src/norms/index.test.ts
✅ Files skipped from review due to trivial changes (1)
- fdm-app/app/components/blocks/gerrit/types.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- fdm-agents/src/agents/gerrit/agent.test.ts
- fdm-agents/src/agents/gerrit/agent.ts
- fdm-agents/src/tools/fertilizer-planner/index.test.ts
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
New Features
Documentation