chore: ratchet weak public API types - #304
Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Endpoint auditContract: pinned release artifact
Actionable client-only calls (0)none Actionable server-only operations (7)
Documented non-divergences (13)Client calls intentionally absent from the filtered contract (11)
Reason: Operational Agent Server endpoints intentionally excluded from the filtered public release artifact.
Reason: Client-ahead API stacked on the pending Agent Server meta-profiles implementation.
Reason: Client-ahead API stacked on the pending Agent Server pre-flight LLM validation endpoint. Server operations covered by an exposed browser URL (2)
Reason: RemoteWorkspace.startWorkspaceSession exposes these authenticated URLs for browser iframe and file requests; they are not HttpClient method calls. |
…ings' into feat/oss-6127-public-type-ratchet
b7dc267 to
dfe83a5
Compare
…type-ratchet Resolve merge conflicts against main: - Adopt main's generated Agent Server schema (1.40.0) and the merged canonical MCP settings operations (endpoint-based create/patch/delete). - Keep the PR's Canonical* type aliases and the public-type ratchet. - Refresh config/public-type-budget.json for the events/types refactor (interface -> type aliases) merged from main. Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
This PR introduces a repository-wide weak-type ratchet over the built public declaration graph, replaces reachable public any with unknown, and adds runtime narrowing for remote full-state events. I reviewed the diff across all 11 files, the budget config, the checker script, and the remote-state runtime changes.
No material issues found. The change is well-structured and the type tightenings are correct.
Key observations
-
remote-state.tsruntime narrowing is a genuine improvement. The previousObject.assign(this.cachedState, event.value)silently coerced non-object primitives (e.g. a string value would spread its characters as numeric-key properties into the cache). The newisRecordguard throws a descriptive error before any mutation, leaving the cache intact. TheAsyncLock.acquirefinallyblock releases the lock on the throw, and the new test verifies post-throw cache integrity (getExecutionStatusstill resolves). Correct. -
normalizeFullStaterewrite is type-safe. Replacing(info as any).full_statewith a typed intersectionConversationInfo & { full_state?: ConversationInfo }preserves the unwrap behavior without weakening types. Good. -
getWorkspace()return type (ConversationInfo['workspace']=unknown) andmodelDump()(Record<string, unknown>) are correct tightenings fromany. Callers depending on the oldanyergonomics will now get compile-time narrowing requirements, which is the intended ratchet effect. -
Budget tooling is sound. The checker keys sites by
module::semantic-path::kindwith occurrence dedup, fingerprints the containing type node text, excludesdist/generated/, and enforces add/widen/change/remove failures. The self-test covers the four comparison cases plus budget-document validation.check:public-type-budgetbuilds first so it always scans fresh.d.tsoutput. -
Canonical MCP type aliases in
clients.ts/index.tsare pure type-level re-exports with no runtime impact. -
CI/pre-commit integration runs both the self-test and the budget check, matching the documented workflow.
Risk assessment
Low risk. This is a chore/type-safety PR. The only runtime behavioral change (updateStateFromEvent now throws on non-object full-state values instead of silently corrupting the cache) is strictly more defensive and is covered by a regression test. No public API signatures change beyond type tightening, and generated declarations remain governed by the separate OpenAPI drift check.
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste — Clean, pragmatic ratchet design. All 15 CI checks pass, including the new public-type-budget job.
The ratchet approach is the right call here: check in the current weak-type budget as a baseline, fail CI on any regression, and let individual PRs shrink the budget incrementally. The script is well-structured — traversal, fingerprinting, validation, and comparison are cleanly separated; the --print-current mode is clearly an inspection aid, not an auto-updater; and the built-in self-test covers compareBudget without requiring any build infrastructure.
Two small suggestions noted inline, neither blocking.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
The runtime changes inremote-state.tsare strictly type-tightening:any→unknownwith explicitisRecordguards. The only behavioral delta is thatupdateStateFromEventnow throws on a non-object__full_state__value rather than silently no-oping — which is the correct fail-fast behavior. All CI checks pass.
VERDICT:
✅ Worth merging: Core logic is sound.
KEY INSIGHT:
Centralising full_state unwrapping in getConversationInfo() via normalizeFullState() is the right simplification — it eliminates the need for every accessor to know about the wrapper format, and makes the cache-hit path as safe as the fetch path.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing. See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
| const group = groups.get(key) ?? { | ||
| category, | ||
| reason, | ||
| owner: 'neubig', |
There was a problem hiding this comment.
🟡 Suggestion: printCurrentSites() hardcodes owner: 'neubig'. Since the docs describe this flag as an inspection aid for intentional updates, another contributor running --print-current to capture a new site will get JSON that misattributes ownership if they commit it directly.
Consider reading the owner from an env var or CLI flag, or at minimum add a comment reminding the user to edit before committing:
| owner: 'neubig', | |
| owner: process.env.BUDGET_OWNER ?? 'neubig', |
| id: 'evt-2', | ||
| kind: 'ConversationStateUpdateEvent', | ||
| timestamp: '2024-01-01T00:00:00Z', | ||
| key: '__full_state__', |
There was a problem hiding this comment.
🟡 Suggestion: This uses the raw string '__full_state__' rather than the FULL_STATE_KEY constant (not exported from remote-state.ts). If the constant is ever renamed, the test will silently exercise the wrong code path. Exporting FULL_STATE_KEY and importing it here would make the coupling explicit, or a minimal fix is a comment:
| key: '__full_state__', | |
| key: '__full_state__', // must match FULL_STATE_KEY in remote-state.ts |
|
🚀 Released in v1.39.0. |
HUMAN:
I have not tested this because it's in CI, but we can test after it goes in.
Why
The client previously had no repository-wide guard against new or widened weak
public types. Important domain boundaries could silently degrade even when
generated Agent Server operations remained strong.
Fixes OpenHands/software-agent-sdk#4755
Linear: OSS-6127
Summary
point and check in a reviewed budget of existing
unknownsites.permanently lowering the budget.
anydeclarations withunknown,with runtime narrowing for remote full-state events.
main(including the Agent Server 1.40.0 schema and the mergedcanonical MCP settings operations from feat: add canonical MCP settings operations #302/feat(events): mirror SDK event payloads #318) and refresh the weak-type
budget for the
events/typesrefactor.Issue Number
OpenHands/software-agent-sdk#4755 / OSS-6127
How to Test
npm run buildnpm run test:public-type-budgetnpm run check:public-type-budgetenv -u AGENT_SERVER_URL npm run test:coverage— 18 suites, 304 testsnpm run check:agent-server-api— checked-in contract is current for 1.40.0npm run test:agent-server-api-toolingnpm run test:endpoint-audit-toolingnpm run lint— 0 errorsnpm run format:check— all files formattedLive evidence refresh (2026-08-08, head
63acaf7= final branch commit after "trigger CI")Re-ran the full validation on the current head (Node v26.6.0):
npm ci-> OK.npm run build->tsc && copy-json-assets && rewrite-relative-importsOK.npm run check:public-type-budget-> Public weak-type budget unchanged: 103 sites (0 any, 103 unknown).npm run test:public-type-budget-> self-test passed.env -u AGENT_SERVER_URL npm run test:coverage-> 18 suites, 304 tests passed (includes theremote-stateandagentchanges).npm run lint-> 0 errors, 8 pre-existing warnings (unchanged).npm run format:check-> all files Prettier-clean.Live evidence (AGENT)
Merge conflict resolution against
mainwas validated locally on commitcbfeeff:The budget refresh mirrors main's
events/typesrefactor (handwritteninterfaces became schema-derived type aliases): six stale
interface:budgettuples were removed and two
type:tuples added at the current fingerprints.Video/Screenshots
Not applicable: this adds public type and CI enforcement without changing the
Canvas UI.
Type
Notes
Depends on #302. Public domain contracts, generic transport internals, and
deliberately opaque Cloud proxy payloads have separate budget groups. Generated
Agent Server declarations remain governed by the SDK OpenAPI quality allowlist
and the pinned generated-file drift check.