Handle invalid dashboard inputs without crashing the Streamlit app - #113
Conversation
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe dashboard now reports invalid repository paths and malformed uploaded traces inline. Uploaded traces are stored only after successful parsing. New tests cover validation errors, storage gating, valid uploads, and persistence. ChangesDashboard input validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR adds guarded handling for invalid repository paths and uploaded traces so the dashboard reports errors instead of crashing; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Operator
participant Dashboard
participant TraceParser
participant TraceStore
Operator->>Dashboard: Upload trace bytes
Dashboard->>TraceParser: parse_uploaded_trace(payload)
TraceParser-->>Dashboard: AgentTrace or TraceUploadError
Dashboard-->>Operator: Show error or storage control
Operator->>Dashboard: Store valid trace
Dashboard->>TraceStore: Persist AgentTrace
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@CHANGELOG.md`:
- Around line 102-107: Update the changelog entry to state that parse positions
are shown only for malformed JSON uploads; describe schema-invalid uploads as
displaying the TraceValidationError field and reason without claiming a parse
position.
In `@dashboard/app.py`:
- Around line 458-464: The try/except around build_patch_plan must only handle
repository-path validation failures, not internal ValueError instances from
diagnosis or source indexing. Validate repo_path before calling build_patch_plan
or have path resolution raise a dedicated exception, catch only that exception,
and allow all other failures to remain observable.
In `@tests/test_dashboard_input_validation.py`:
- Around line 85-107: Add coverage in the uploaded-trace validation tests for
invalid UTF-8 bytes, asserting no uncaught app exception and an inline error.
Strengthen test_uploaded_trace_with_malformed_json_reports_the_position to
assert the reported JSON parse error includes its line and column details, while
preserving the existing error-message assertion.
- Around line 85-121: Update the three upload tests around _upload and the
“Store uploaded trace” control: assert the control is absent or unusable for
malformed JSON and schema-invalid uploads, and visible/usable for a valid
upload. In test_a_valid_uploaded_trace_is_accepted, activate the store control
and verify the resulting success state or persisted trace using the existing
test helpers and public behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e6cea28a-f5b8-4b42-8374-76d471806748
📒 Files selected for processing (3)
CHANGELOG.mddashboard/app.pytests/test_dashboard_input_validation.py
dipeshbabu
left a comment
dipeshbabu
left a comment
There was a problem hiding this comment.
Thanks for tackling this — the direction is good, but this is not ready to merge yet. Please address the following before re-requesting review:
-
Fix the new dashboard-test cache leakage (
tests/test_dashboard_input_validation.py). The full CI suite now fails on Python 3.10, 3.13, and 3.14. On 3.13 the existingtests/test_dashboard_quality.py::test_quality_gate_dashboard_reports_empty_fixture_suite_inlinereachesapp.text_area[0]with no text area rendered.dashboard/app.pyuses@st.cache_resourceforload_store(), while each new test pointsAGENTLOOP_SQLITE_PATHat a different temporary DB; the cached store survives across AppTest instances and contaminates later dashboard tests. Add deterministic cache isolation — e.g. an autouse fixture for these AppTests that callsst.cache_resource.clear()before and after each test (or a shared dashboard-test fixture with equivalent cleanup). Then run the full suite and make all Python matrix jobs green, not just the six new tests. -
Restrict the repository-path exception boundary (
dashboard/app.py, Patch Plan).except ValueErrorcurrently wraps the entirebuild_patch_plan(...)call._resolve_repository_path()intentionally raisesValueError, butbuild_patch_plan()continues through diagnosis/source indexing and any later internalValueErrorwould be mislabeled as “Fix the repository path” and hidden byst.stop(). That conflicts with #21’s requirement that unexpected internal failures remain observable. Please either introduce a dedicated repository-path exception (for exampleRepositoryPathError(ValueError)) inagentloop.patchesand catch only that, or validate the repository path in a dedicated helper before callingbuild_patch_plan. Add a regression proving an unrelated/internalValueErroris not converted into a repository-path validation message. -
Finish the upload validation contract in the tests. Add invalid-UTF-8 coverage for the
UnicodeErrorbranch; strengthen malformed-JSON coverage to assert the reported line and column; assertStore uploaded traceis absent/disabled for malformed and schema-invalid uploads; and for a valid upload, click the store control and verify the success state or persisted trace. These are currently unverified public behaviors. -
Meet #21’s shared-helper acceptance criterion. The issue explicitly requires shared parsing/validation helpers to be unit tested. The new trace-upload parsing is still embedded directly in page code. Please extract the upload decode/JSON/schema-validation boundary into a small reusable helper (with structured/safe errors) and unit-test it independently; keep representative AppTest coverage for the page behavior.
-
Correct the changelog wording. Parse line/column is available for malformed JSON only.
TraceValidationErrorreports the schema field/reason, not a JSON parse position.
Once these are fixed, please rerun the complete repository CI matrix. CodeQL, dependency review, Docker, package, and standalone jobs are already green on this head; the Python test jobs are the current blocker.
|
No description provided. |
Replacing this with a shorter review that tags the contributor directly.
dipeshbabu
left a comment
dipeshbabu
left a comment
There was a problem hiding this comment.
@dchaudhari7177 thanks for this. A few things still need fixing before merge:
- Full CI is failing on Python 3.10, 3.13, and 3.14 because the new dashboard AppTests leak the cached Streamlit store between tests. Please clear
st.cache_resourcebefore and after each dashboard test. - Catch only repository-path validation errors. The current
except ValueErrorcan hide unrelated internal errors frombuild_patch_plan(). - Add tests for invalid UTF-8, JSON line/column, invalid uploads hiding the store button, and valid upload persistence.
- Please extract the trace upload parsing into a small helper and unit test it, as required by #21.
- Fix the changelog wording so parse positions are only claimed for malformed JSON.
Once these are fixed and the full CI matrix is green, re-request review.
…solate the cache Five changes requested on dipeshbabu#113. Cache isolation. dashboard/app.py builds its store under @st.cache_resource, whose key is the load_store function rather than the database path, so the store outlived each AppTest and the second test in a process read the first test's database -- by which time that tmp_path was gone. An autouse fixture clears st.cache_resource on both sides of every test, so neither a leftover from an earlier module nor one this module leaves can decide a result. This is what reddened 3.10, 3.13 and 3.14. Narrower catch. _resolve_repository_path raised a bare ValueError, so the Patch Plan page's except ValueError also swallowed anything build_patch_plan raised beneath it. It now raises RepositoryPathError, a ValueError subclass (matching TraceValidationError), exported from the package; the page catches only that, and a bug in planning surfaces instead of being reported as a path the operator should fix. Extracted parser. The three ways an upload can be unusable -- not UTF-8, not JSON, not a trace -- arrive from three layers, and translating them inline left the only copy of that logic inside a page reachable only through AppTest. dashboard/trace_upload.py holds parse_uploaded_trace() raising TraceUploadError, unit tested in tests/test_dashboard_trace_upload.py (6 cases), which is what dipeshbabu#21 asks for. It also closes a gap the inline version had: json.loads returns a list for "[]", and from_dict then failed on subscripting rather than on the schema. Tests. Added invalid UTF-8, the line-3-column-1 assertion for malformed JSON, a parametrized case pinning that all three invalid uploads leave the store button unrendered, and one that clicks through a valid upload and asserts the trace is readable from the store afterwards. 11 AppTest cases and 6 unit cases pass locally. Changelog. The entry claimed a parse position for schema violations too; positions are now claimed for malformed JSON alone, with the other two located by byte offset and by field. tests/test_patch_plan.py has two failures (tool_oscillation missing from SUPPORTED_PATCH_TYPES) that reproduce unchanged on upstream/main and are untouched by this branch. Refs dipeshbabu#21 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks for the detailed review — all five are addressed in b51375d. Cache leak (the CI failure). You were right about the mechanism. Narrower catch. Extracted parser (#21). Tests. Added invalid UTF-8, an explicit Changelog. It claimed a parse position for schema violations too. Positions are now claimed for malformed JSON alone — the other two are located by byte offset and by field, and neither has a line/column to give. Local: 11 AppTest cases + 6 unit cases pass, One thing to flag, unrelated to this branch: Re-requesting review. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/test_dashboard_input_validation.py`:
- Around line 152-171: Add the zero-byte payload case to
tests/test_dashboard_input_validation.py lines 152-171, preserving assertions
that an inline error is shown and no “Store uploaded trace” button is rendered.
Add a parser-layer test in tests/test_dashboard_trace_upload.py lines 38-44
asserting that b"" raises TraceUploadError with the expected JSON line and
column location.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0f796169-0a76-4139-85db-c342cf9dfeb3
📒 Files selected for processing (7)
CHANGELOG.mdagentloop/__init__.pyagentloop/patches.pydashboard/app.pydashboard/trace_upload.pytests/test_dashboard_input_validation.pytests/test_dashboard_trace_upload.py
|
All five points are addressed — b51375d for the first four, a6166ea for the zero-byte case CodeRabbit raised afterwards. Walking them in your order: 1. Cache leakage. An autouse fixture in the dashboard tests clears 2. Exception boundary. 3. Upload validation contract. Now covered: invalid UTF-8, malformed JSON asserting line and column, schema-invalid, JSON that parses to a non-object, and the zero-byte file. A parametrised test asserts the store button is unrendered for every invalid payload; the valid path asserts it is rendered, clicks it, and checks the trace reaches the store under the shown 4. Shared helper. The decode/JSON/schema boundary moved to 5. Changelog. Reworded: parse position is claimed for malformed JSON only; schema violations are described as reporting the One design note worth flagging, since it's the one place I made a judgement call: Re-requesting review. |
Streamlit reruns the whole script on every widget change, so a control that parses or executes user-controlled input has no natural moment to wait for the input to be finished. Two paths reached the library directly: - Patch Plan passed a free-text repository path to build_patch_plan on every rerun. _resolve_repository_path raises ValueError for a path that does not exist or escapes the allowed root, so a half-typed path replaced the page with a traceback before the operator could finish typing it. - Ingest decoded an uploaded file with json.loads and AgentTrace.from_dict with nothing around them, so malformed JSON or a schema violation did the same on the Ingest page. Both now report inline, following the pattern the quality-fixture path already uses: JSONDecodeError reported with its position, then the narrower UnicodeError/TraceValidationError pair. The upload's store button moves into the else branch so it cannot act on a trace that failed to parse. Six tests via streamlit.testing AppTest: both repository-path failures, both upload failures, and a control for each that the valid path still works. Revert-verified -- the four failure cases fail without the guards, the two controls pass either way. Closes dipeshbabu#21
…solate the cache Five changes requested on dipeshbabu#113. Cache isolation. dashboard/app.py builds its store under @st.cache_resource, whose key is the load_store function rather than the database path, so the store outlived each AppTest and the second test in a process read the first test's database -- by which time that tmp_path was gone. An autouse fixture clears st.cache_resource on both sides of every test, so neither a leftover from an earlier module nor one this module leaves can decide a result. This is what reddened 3.10, 3.13 and 3.14. Narrower catch. _resolve_repository_path raised a bare ValueError, so the Patch Plan page's except ValueError also swallowed anything build_patch_plan raised beneath it. It now raises RepositoryPathError, a ValueError subclass (matching TraceValidationError), exported from the package; the page catches only that, and a bug in planning surfaces instead of being reported as a path the operator should fix. Extracted parser. The three ways an upload can be unusable -- not UTF-8, not JSON, not a trace -- arrive from three layers, and translating them inline left the only copy of that logic inside a page reachable only through AppTest. dashboard/trace_upload.py holds parse_uploaded_trace() raising TraceUploadError, unit tested in tests/test_dashboard_trace_upload.py (6 cases), which is what dipeshbabu#21 asks for. It also closes a gap the inline version had: json.loads returns a list for "[]", and from_dict then failed on subscripting rather than on the schema. Tests. Added invalid UTF-8, the line-3-column-1 assertion for malformed JSON, a parametrized case pinning that all three invalid uploads leave the store button unrendered, and one that clicks through a valid upload and asserts the trace is readable from the store afterwards. 11 AppTest cases and 6 unit cases pass locally. Changelog. The entry claimed a parse position for schema violations too; positions are now claimed for malformed JSON alone, with the other two located by byte offset and by field. tests/test_patch_plan.py has two failures (tool_oscillation missing from SUPPORTED_PATCH_TYPES) that reproduce unchanged on upstream/main and are untouched by this branch. Refs dipeshbabu#21 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An empty file is one click away in any file picker, and it takes a different path through the parser than the other rejections: empty bytes are valid UTF-8, so it falls past the decode layer and fails as JSON at the first character. That position is real rather than fabricated, so the page test asserts it verbatim. Parser-layer test pins the message; the page-level parametrised case pins that the store button stays unrendered, same as the other invalid payloads.
a6166ea to
7e0419d
Compare
dipeshbabu
left a comment
dipeshbabu
left a comment
There was a problem hiding this comment.
@dchaudhari7177 looks good now. The requested fixes are in and all checks pass. Thanks for following through on this.
2 participants
Closes #21.
What changed
Two of the three paths in the issue were still unguarded. The quality-fixture path (
json.loadsat what is now line 615) has since been fixed and already has thetry/st.errortreatment — I followed its shape rather than inventing a second one.build_patch_plan(trace, repo_path=…)ValueErrorfrom_resolve_repository_path— "must identify an existing directory" or "must remain within the allowed root"st.error("Fix the repository path: …")thenst.stop()AgentTrace.from_dict(json.loads(…))JSONDecodeError,UnicodeError, orTraceValidationErrorst.errornaming the parse position, or the validation messageNote the line numbers in the issue have shifted; the call sites are now
dashboard/app.py:457and:739.Two details worth calling out:
elsebranch. Previously it sat in the same block as the parse, so a failed parse would leave a "Store uploaded trace" button referring to atracethat was never bound. Now it only exists when there is something to store.st.stop()rather than wrapping 36 lines intry/else. The Patch Plan body that consumesplanruns to the end of the page section. Re-indenting it would have made the diff mostly whitespace and hidden the actual change.st.stop()ends the rerun with the error rendered, which is the same observable result.Tests
Six, in
tests/test_dashboard_input_validation.py, usingstreamlit.testing.v1.AppTest— the patterntest_dashboard_quality.pyalready establishes:../../..) — the traversal refusal should be legible, not a crashRevert-verified: with
dashboard/app.pyreverted, the four failure cases fail and both controls pass — so the controls are not quietly passing for the wrong reason.One test-authoring note, since it nearly produced a test that passed against nothing:
app.text_input[-1]is the sidebar Project field, not the repository path. Setting it switched project, emptied the page, and the assertion then held against a page with no content at all. The helper selects by label instead, and there's a comment saying why.Verification
Full suite is 51 pre-existing failures on this machine, unrelated to this change — my Python 3.12 environment has newer FastAPI/Starlette than the project expects, so
tests/test_client.py,test_server.pyandtest_migrations.pydo not even collect (Router.__init__() got an unexpected keyword). Rather than quote a total, I diffed the failure sets:Nothing else moved.
Separately: running the dashboard tests needs a newer Streamlit than
pyproject.toml'sstreamlit>=1.34.0floor.st.dataframe(df, width="stretch")raisesTypeError: 'str' object cannot be interpreted as an integeron 1.43.2 — the string form ofwidthis much newer. The existing dashboard tests don't catch it because they never render a populated dataframe. That looks like a real floor bug, but it is not this issue, so I left it alone; happy to file it.ruff checkandruff formatclean at the project'sline-length = 100. Changelog entry added underUnreleased → Fixed.AI assistance
Substantial, and disclosed per CONTRIBUTING: written with Claude Code (Claude Opus 5). It located the call sites, wrote the guards and the tests, and drafted this description. I ran the suite, the revert check and the before/after failure diff, and traced the exception types back to
_resolve_repository_pathandTraceValidationErrorin the source rather than assuming them.Summary by CodeRabbit
Bug Fixes
Documentation