Doctor: expose tool result cap findings#97500
Conversation
|
@clawsweeper review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
Codex review: needs maintainer review before merge. Reviewed July 2, 2026, 12:41 AM ET / 04:41 UTC. Summary Reproducibility: not applicable. this is a feature PR rather than a bug report. The changed behavior is checkable with Review metrics: 1 noteworthy metric.
Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the focused structured check after maintainer review and required checks, preserving legacy Doctor note output and the reviewed opt-in lint semantics. Do we have a high-confidence way to reproduce the issue? Not applicable; this is a feature PR rather than a bug report. The changed behavior is checkable with Is this the best way to solve the issue? Yes; this is the best observed shape because it reuses existing cap advice, model/context-window helpers, agent context-limit inheritance, and default-disabled Doctor lint selection instead of adding a parallel config interpretation. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 8c7ac9b9b9df. Label changesLabel justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
|
@clawsweeper review Addressed the P2 inherited-default target enumeration finding in 2fe869c. Structured lint now includes agents inheriting defaults caps and has a regression for an agent-specific larger model. Validation: focused Vitest 57 tests, changed-file oxlint, tsgo:core, local child check:changed, SDK surface, diff check. |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@clawsweeper review Addressed the P2 per-agent path-shape finding in 5f671c8. Explicit per-agent tool-result-cap findings now use agents.list..contextLimits.toolResultMaxChars / target agents.list.; inherited defaults stay on agents.defaults.contextLimits.toolResultMaxChars. Validation: focused Vitest 57 tests, changed-file oxlint, tsgo:core, local child check:changed, SDK surface, diff check. |
|
🦞🧹 I asked ClawSweeper to review this item again. |
galiniliev
left a comment
There was a problem hiding this comment.
This PR adds a default-disabled structured core/doctor/tool-result-cap lint check for explicit toolResultMaxChars settings, while keeping the existing human Doctor note path as the normal openclaw doctor behavior. That matters because the new JSON finding surface is now an operator-facing contract for path, target, and fix-hint metadata.
LOC: +366/-47 (4 files)
Findings: No blocking findings.
Bug/behavior: feature PR for Doctor structured lint output; no shipped regression being repaired. The affected surface is Doctor lint/contribution registration and tool-result cap advice formatting.
Best-fix verdict: best. The PR reuses the existing cap calculations and Doctor advice helper instead of adding a parallel interpretation of toolResultMaxChars, and the structured check is opt-in through the existing defaultEnabled: false lint-selection contract.
Alternatives considered:
- Put the check in
doctor-core-checks.ts: rejected because this behavior already belongs to the ordereddoctor:tool-result-capcontribution and has a legacy Doctor run path there. - Make
doctor --lintrun it by default: rejected because this is advisory/no-repair guidance and would turn existing explicit caps into default warning exits. - Emit only a defaults-level finding: rejected because configured agents can inherit the default cap while resolving different models/context windows, and the current implementation keeps the legacy per-target enumeration.
Code read: src/flows/doctor-health-contributions.ts, src/flows/doctor-tool-result-cap-advice.ts, src/flows/doctor-lint-flow.ts, src/commands/doctor-lint.ts, src/config/schema.help.ts, src/agents/agent-scope-config.ts, src/agents/context-window-guard.ts, src/agents/model-selection.ts, src/agents/model-catalog.ts, src/agents/embedded-agent-runner/tool-result-truncation.ts, and adjacent tests.
Evidence map: changed surface is the Doctor contribution plus cap-advice helper; entry point is openclaw doctor --lint; owner boundary is core Doctor health contributions; caller checked through runDoctorLintCli and runDoctorLintChecks; callees checked through model selection/catalog, context-window resolution, agent context-limit merging, and runtime cap calculation; sibling invariant checked against existing default-disabled Doctor lint checks and config path help; current origin/main only has legacy note output for this contribution.
Validation run:
node scripts/run-vitest.mjs run src/flows/doctor-tool-result-cap-advice.test.ts src/flows/doctor-health-contributions.test.ts --reporter=verbose-> 57 passed.- Real CLI probe with isolated
OPENCLAW_CONFIG_PATHand--only core/doctor/tool-result-capemitted warning findings foragents.defaults.contextLimits.toolResultMaxCharsand inheritedtarget: agents.list.writer; exit 1 was expected because warnings meet the default severity threshold. The probe built stale localdistfirst and left the worktree clean.
Remaining uncertainty: I did not run the broader changed gate or remote/Testbox proof in this pass; the PR body already reports changed-gate and type/lint proof for this exact head.
5f671c8 to
1033813
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
1033813 to
214ac9c
Compare
1347ee5 to
1a533f7
Compare
|
Merged via squash.
|
Summary
core/doctor/tool-result-captoolResultMaxCharsadvice into warning findings with stable path/target/fixHint metadataopenclaw doctornote output as the authority for normal Doctor runsPublic contract
defaultEnabled: false; defaultdoctor --lintskips it while--alland--only core/doctor/tool-result-capinclude it.Real behavior proof
Source CLI proof with isolated
OPENCLAW_CONFIG_PATHcontainingagents.defaults.contextLimits.toolResultMaxChars = 16000:pnpm --silent openclaw doctor --lint --json:checksRun: 25,checksSkipped: 8,toolResultCapFindings: 0pnpm --silent openclaw doctor --lint --json --only core/doctor/tool-result-cap:checksRun: 1,checksSkipped: 32, finding pathagents.defaults.contextLimits.toolResultMaxChars, targetagents.defaults, severitywarningReview updates
agents.list.<id>.contextLimits.toolResultMaxCharsandtarget: agents.list.<id>; inherited-default findings remain anchored toagents.defaults.contextLimits.toolResultMaxCharswith per-agent targets.Rebase refresh - 2026-07-01 after #97496
Rebased onto current
origin/mainafter #97496 merged and refreshed the PR head toa9d3dfd9f58d087abb8bc13414ecd44534a1516e.Fresh validation after rebase:
./node_modules/.bin/oxfmt --check src/flows/doctor-health-contributions.ts src/flows/doctor-health-contributions.test.ts src/flows/doctor-tool-result-cap-advice.ts src/flows/doctor-tool-result-cap-advice.test.ts./node_modules/.bin/oxlint src/flows/doctor-health-contributions.ts src/flows/doctor-health-contributions.test.ts src/flows/doctor-tool-result-cap-advice.ts src/flows/doctor-tool-result-cap-advice.test.tsnode scripts/run-vitest.mjs src/flows/doctor-tool-result-cap-advice.test.ts src/flows/doctor-health-contributions.test.ts(63 tests)node scripts/run-tsgo.mjs -p tsconfig.core.json --incremental --tsBuildInfoFile .artifacts/tsgo-cache/core.tsbuildinfonode scripts/run-tsgo.mjs -p test/tsconfig/tsconfig.core.test.json --incremental --tsBuildInfoFile .artifacts/tsgo-cache/core-test.tsbuildinfonode scripts/plugin-sdk-surface-report.mjs --checknode scripts/sync-plugin-sdk-exports.mjs --checkgit diff --checkNotes
pnpm check:changedfirst tried to delegate to Blacksmith Testbox and was blocked by the local Crabbox binary version (0.15.0, requires>=0.22.0), so the prior validation used the documented local child path directly withOPENCLAW_CHECK_CHANGED_REMOTE_CHILD=1.