Code smell remediation - #1
Conversation
📝 WalkthroughWalkthroughThis PR consolidates duplicated SDK integration patterns across Go, Python, and Node by introducing shared typed response models, normalization helpers, and contract fixtures. It refactors HTTP client logic, adds repository hygiene guardrails via CI workflows and cleanup targets, and establishes a structured code-smell remediation roadmap with planning documents. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
🧹 Nitpick comments (4)
python/tests/test_client.py (1)
449-455: Prefix unused variable with underscore.Static analysis correctly flags
fake_httpas unused. Prefix it to indicate intentional disuse.def test_chat_history_list_method(self) -> None: - client, fake_http = self._new_client() + client, _fake_http = self._new_client()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/tests/test_client.py` around lines 449 - 455, The test assigns an unused variable `fake_http` from _new_client(), causing static analysis warnings; rename it to `_fake_http` (or prefix with an underscore) in the `test_chat_history_list_method` so the call stays the same but the linter recognizes the variable as intentionally unused; update the variable name wherever it's referenced in that test (here only in the assignment) to `_fake_http`.node/simpleflow_sdk/index.js (1)
650-660: Consider simplifyingsampledForEventandsampledForEnvelope.Both values are computed identically (line 658-659). While separating them provides future flexibility, the Python SDK uses a single
sampledvalue. Consider unifying for cross-SDK consistency.- sampledForEvent, - sampledForEnvelope: sampledForEvent, + sampled: sampledForEvent,Then update lines 620 and 633 to use
context.sampled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/simpleflow_sdk/index.js` around lines 650 - 660, Unify the duplicate sampling fields by replacing sampledForEvent and sampledForEnvelope with a single sampled property: compute sampled once (currently computed into sampledForEvent) using typeof telemetry.sampled === "boolean" ? telemetry.sampled : true and return it as sampled, then update places that read context.sampled (previously using sampledForEvent/sampledForEnvelope) — specifically change the usages in the code that reference sampledForEvent and sampledForEnvelope (e.g., the callers around the former lines where context is built and at the two call sites you noted) to read context.sampled so the SDK matches the Python single-sampled convention.python/simpleflow_sdk/response_models.py (1)
92-97: Missing trailing newline.The file should end with a newline for POSIX compliance.
class ChatSession(TypedDict, total=False): chat_id: str status: str agent_id: str user_id: str metadata: dict[str, Any] +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python/simpleflow_sdk/response_models.py` around lines 92 - 97, The file ends without a POSIX newline; add a single trailing newline at EOF so the ChatSession TypedDict declaration (class ChatSession) is followed by a newline character. Ensure the file is saved with a final '\n' after the last line so tools expecting POSIX-compliant files recognize the newline.go/simpleflow/client_test.go (1)
333-364: Unchecked type assertions may cause test panics on fixture format changes.Multiple type assertions (e.g.,
fixture["expected_event"].(map[string]any),payload["usage"].(map[string]any)) lack safety checks. If the fixture format drifts or a key is missing, the test will panic instead of providing a clear failure message.🛡️ Suggested defensive approach
- expectedEvent := fixture["expected_event"].(map[string]any) + expectedEvent, ok := fixture["expected_event"].(map[string]any) + if !ok { + t.Fatalf("expected_event is not a map") + }Apply similar checks for
expectedPayload,payload,usage,expectedUsage, andmodelUsageassertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go/simpleflow/client_test.go` around lines 333 - 364, The test currently uses unchecked type assertions (e.g., expectedEvent := fixture["expected_event"].(map[string]any), payload := captured["payload"].(map[string]any), usage := payload["usage"].(map[string]any), expectedUsage := expectedPayload["usage"].(map[string]any), modelUsage := payload["model_usage"].([]any)) which can panic on malformed fixtures; replace these with safe checks (use the "comma ok" form) and call t.Fatalf with clear messages when a key is missing or the type is wrong, and for modelUsage validate it is a []any and has at least one element before accessing [0]; ensure you check captured["payload"], fixture["expected_payload"], and all nested keys exist and have expected types (expectedEvent, expectedPayload, payload, usage, expectedUsage, modelUsage) before comparing values so failures report descriptive errors instead of panics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/sdk-integration-common.md`:
- Around line 30-33: The docs currently only list the user bearer env variable
`SIMPLEFLOW_USER_BEARER`; add an explicit API-key env variable entry (e.g.,
`SIMPLEFLOW_API_KEY` or `SIMPLEFLOW_AG_API_KEY`) alongside
`SIMPLEFLOW_USER_BEARER` to show chat accepts agk_* API keys or user bearers;
update the User-scoped APIs section to include that API-key env var name and a
short example note indicating it accepts agk_* keys for chat use cases so
readers aren’t ambiguous about which env var to set.
- Around line 44-45: Update the shared sequence wording to use Python snake_case
method names instead of Node-style names so Python users can copy/paste
directly: replace Node-style references like withTelemetry(...).emitSpan(...)
and writeEventFromWorkflowResult with the Python equivalents
with_telemetry(...).emit_span(...) and write_event_from_workflow_result (or
include both language variants side-by-side). Ensure the same change is applied
to the chat history guidance to mention using a user bearer token with the
language-appropriate API names (e.g., Python snake_case).
In `@Makefile`:
- Around line 68-73: The Makefile's clean target currently removes various build
directories but omits Python bytecode artifacts; update the clean recipe for the
clean target to also remove Python __pycache__ directories and *.pyc files
(e.g., add rm -rf "**/__pycache__" "**/*.pyc" and/or remove __pycache__ and
*.pyc under the python/ and any package dirs referenced by the existing
patterns) so Python bytecode is cleaned alongside python/dist, python/build, and
egg-info; apply the changes within the existing clean target (referencing the
Makefile target name clean) to avoid leaving generated Python artifacts behind.
---
Nitpick comments:
In `@go/simpleflow/client_test.go`:
- Around line 333-364: The test currently uses unchecked type assertions (e.g.,
expectedEvent := fixture["expected_event"].(map[string]any), payload :=
captured["payload"].(map[string]any), usage :=
payload["usage"].(map[string]any), expectedUsage :=
expectedPayload["usage"].(map[string]any), modelUsage :=
payload["model_usage"].([]any)) which can panic on malformed fixtures; replace
these with safe checks (use the "comma ok" form) and call t.Fatalf with clear
messages when a key is missing or the type is wrong, and for modelUsage validate
it is a []any and has at least one element before accessing [0]; ensure you
check captured["payload"], fixture["expected_payload"], and all nested keys
exist and have expected types (expectedEvent, expectedPayload, payload, usage,
expectedUsage, modelUsage) before comparing values so failures report
descriptive errors instead of panics.
In `@node/simpleflow_sdk/index.js`:
- Around line 650-660: Unify the duplicate sampling fields by replacing
sampledForEvent and sampledForEnvelope with a single sampled property: compute
sampled once (currently computed into sampledForEvent) using typeof
telemetry.sampled === "boolean" ? telemetry.sampled : true and return it as
sampled, then update places that read context.sampled (previously using
sampledForEvent/sampledForEnvelope) — specifically change the usages in the code
that reference sampledForEvent and sampledForEnvelope (e.g., the callers around
the former lines where context is built and at the two call sites you noted) to
read context.sampled so the SDK matches the Python single-sampled convention.
In `@python/simpleflow_sdk/response_models.py`:
- Around line 92-97: The file ends without a POSIX newline; add a single
trailing newline at EOF so the ChatSession TypedDict declaration (class
ChatSession) is followed by a newline character. Ensure the file is saved with a
final '\n' after the last line so tools expecting POSIX-compliant files
recognize the newline.
In `@python/tests/test_client.py`:
- Around line 449-455: The test assigns an unused variable `fake_http` from
_new_client(), causing static analysis warnings; rename it to `_fake_http` (or
prefix with an underscore) in the `test_chat_history_list_method` so the call
stays the same but the linter recognizes the variable as intentionally unused;
update the variable name wherever it's referenced in that test (here only in the
assignment) to `_fake_http`.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4f5f5e31-adc9-4e45-81ac-220825772786
📒 Files selected for processing (25)
.github/workflows/repo-hygiene.yml.gitignoreCNAMEMakefileREADME.mdcode-review/code_smell_fix_plan.mdcode-review/code_smell_pr_execution_plan.mdcode-review/code_smell_remediation_technical_plan.mdcode-review/code_smell_report.mdcontracts/runtime-registration/action_path_cases.jsoncontracts/telemetry-envelope-v1/workflow_basic.jsondocs/index.mddocs/sdk-go-integration.mddocs/sdk-integration-common.mddocs/sdk-node-integration.mddocs/sdk-python-integration.mdgo/simpleflow/client.gogo/simpleflow/client_test.gogo/simpleflow/response_models.gonode/simpleflow_sdk/index.jsnode/simpleflow_sdk/tests/client.test.jspython/simpleflow_sdk/__init__.pypython/simpleflow_sdk/client.pypython/simpleflow_sdk/response_models.pypython/tests/test_client.py
💤 Files with no reviewable changes (1)
- CNAME
| User-scoped APIs: | ||
|
|
||
| - `SIMPLEFLOW_USER_BEARER` | ||
|
|
There was a problem hiding this comment.
Document the API-key env path alongside bearer for chat use cases.
Line 9 says chat accepts agk_* API key or user bearer, but Line 30–33 only documents SIMPLEFLOW_USER_BEARER. Add an explicit API-key env variable example to avoid ambiguity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/sdk-integration-common.md` around lines 30 - 33, The docs currently only
list the user bearer env variable `SIMPLEFLOW_USER_BEARER`; add an explicit
API-key env variable entry (e.g., `SIMPLEFLOW_API_KEY` or
`SIMPLEFLOW_AG_API_KEY`) alongside `SIMPLEFLOW_USER_BEARER` to show chat accepts
agk_* API keys or user bearers; update the User-scoped APIs section to include
that API-key env var name and a short example note indicating it accepts agk_*
keys for chat use cases so readers aren’t ambiguous about which env var to set.
| 5. Optionally emit span telemetry with `withTelemetry(...).emitSpan(...)`. | ||
| 6. Use chat history APIs with user bearer token when needed. |
There was a problem hiding this comment.
Use language-specific method names in the shared sequence.
Line 44 and Line 45 use Node-style names, but Python uses snake_case (write_event_from_workflow_result, with_telemetry(...).emit_span(...)). This can break direct copy/paste for Python integrations.
Suggested wording
-4. Emit canonical telemetry via `writeEventFromWorkflowResult(...)`.
-5. Optionally emit span telemetry with `withTelemetry(...).emitSpan(...)`.
+4. Emit canonical telemetry (Node: `writeEventFromWorkflowResult(...)`, Python: `write_event_from_workflow_result(...)`, Go: `WriteEventFromWorkflowResult(...)`).
+5. Optionally emit span telemetry (Node: `withTelemetry(...).emitSpan(...)`, Python: `with_telemetry(...).emit_span(...)`, Go: `WithTelemetry(...).EmitSpan(...)`).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/sdk-integration-common.md` around lines 44 - 45, Update the shared
sequence wording to use Python snake_case method names instead of Node-style
names so Python users can copy/paste directly: replace Node-style references
like withTelemetry(...).emitSpan(...) and writeEventFromWorkflowResult with the
Python equivalents with_telemetry(...).emit_span(...) and
write_event_from_workflow_result (or include both language variants
side-by-side). Ensure the same change is applied to the chat history guidance to
mention using a user bearer token with the language-appropriate API names (e.g.,
Python snake_case).
| clean: | ||
| rm -rf "docs/node_modules" "docs/.vitepress/dist" "docs/.vitepress/cache" | ||
| rm -rf "examples/simpleflow-hr-agent/node_modules" ".opencode/node_modules" | ||
| rm -rf "python/dist" "python/build" "python"/*.egg-info | ||
| rm -rf "node/simpleflow_sdk/node_modules" | ||
|
|
There was a problem hiding this comment.
Extend clean to remove Python bytecode artifacts too.
At Line 68–73, clean omits __pycache__/ and *.pyc, but CI blocks those as generated artifacts. This leaves a hygiene gap for local cleanup.
Suggested patch
clean:
rm -rf "docs/node_modules" "docs/.vitepress/dist" "docs/.vitepress/cache"
rm -rf "examples/simpleflow-hr-agent/node_modules" ".opencode/node_modules"
rm -rf "python/dist" "python/build" "python"/*.egg-info
rm -rf "node/simpleflow_sdk/node_modules"
+ find . -type d -name "__pycache__" -prune -exec rm -rf {} +
+ find . -type f -name "*.pyc" -delete📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| clean: | |
| rm -rf "docs/node_modules" "docs/.vitepress/dist" "docs/.vitepress/cache" | |
| rm -rf "examples/simpleflow-hr-agent/node_modules" ".opencode/node_modules" | |
| rm -rf "python/dist" "python/build" "python"/*.egg-info | |
| rm -rf "node/simpleflow_sdk/node_modules" | |
| clean: | |
| rm -rf "docs/node_modules" "docs/.vitepress/dist" "docs/.vitepress/cache" | |
| rm -rf "examples/simpleflow-hr-agent/node_modules" ".opencode/node_modules" | |
| rm -rf "python/dist" "python/build" "python"/*.egg-info | |
| rm -rf "node/simpleflow_sdk/node_modules" | |
| find . -type d -name "__pycache__" -prune -exec rm -rf {} + | |
| find . -type f -name "*.pyc" -delete |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 68 - 73, The Makefile's clean target currently removes
various build directories but omits Python bytecode artifacts; update the clean
recipe for the clean target to also remove Python __pycache__ directories and
*.pyc files (e.g., add rm -rf "**/__pycache__" "**/*.pyc" and/or remove
__pycache__ and *.pyc under the python/ and any package dirs referenced by the
existing patterns) so Python bytecode is cleaned alongside python/dist,
python/build, and egg-info; apply the changes within the existing clean target
(referencing the Makefile target name clean) to avoid leaving generated Python
artifacts behind.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc0568ca99
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (typeof source.schema_version === "string") result.schema_version = source.schema_version; | ||
| if (typeof source.run_id === "string") result.run_id = source.run_id; | ||
| if (typeof source.status === "string") result.status = source.status; | ||
| if (source.output && typeof source.output === "object" && !Array.isArray(source.output)) result.output = source.output; |
There was a problem hiding this comment.
invoke() now routes through normalizeInvokeResult, but that normalizer only keeps output when it is a non-array object. If the runtime returns a valid scalar/array output (for example a string reply or list payload), the SDK silently drops output from the returned object, which is a behavior regression from the previous raw response passthrough and breaks callers that read response.output.
Useful? React with 👍 / 👎.
| if isinstance(value.get("output"), dict): | ||
| result["output"] = value["output"] |
There was a problem hiding this comment.
The new invoke_typed() normalization only includes output when it is a dict, and invoke() now depends on that path. Any valid invoke response where output is a scalar/array/null is returned without an output key, which is a backward-incompatible behavior change from the prior invoke() passthrough and can make successful runtime results look empty.
Useful? React with 👍 / 👎.
| raise TypeError( | ||
| "simpleflow sdk payload error: payload must be a dataclass, dict, or None" | ||
| ) | ||
| return {field.name: getattr(payload, field.name) for field in fields(payload)} |
There was a problem hiding this comment.
Switching from asdict to a shallow field copy leaves nested dataclass instances unconverted in payloads. Those nested objects are then forwarded to HTTP JSON encoding, which raises a serialization error for dataclass instances; previously this worked because asdict recursively converted nested dataclasses. This introduces runtime failures for nested dataclass request bodies.
Useful? React with 👍 / 👎.
1 participant
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests
Chores