4.8 KiB
4.8 KiB
Residual Review Findings — feat/bitable-enhancement
Source
- Review: ce-code-review (mode:agent) on 2026-07-03 (initial pass) + 2026-07-04 (final pre-merge pass)
- Branch: feat/bitable-enhancement
- Commits reviewed: e1cf073..3fdd29d (6 U-ID commits + simplification + residual + frontend rebuild)
- Overall assessment: PASS WITH FINDINGS — initial 0 P0, 0 P1, 2 P2, 3 P3; final 0 P0, 0 P1, 4 P2 (2 new), 3 P3. Ready to merge.
Residual Findings (deferred to downstream resolver)
DR-1: Pre-existing text() SQL calls in repository.py (P2, security-lens, confidence HIGH)
- File:
src/agentkit/bitable/repository.pyline 660, 778-779 - Description: Pre-existing
text()calls with potential SQL injection risk. These calls exist onorigin/mainand were NOT introduced by this branch (verified via git diff). The newdelete_viewmethod (line 467-472) uses ORMdelete(ViewModel).where(...)and is safe. - Suggested fix: Migrate
text()calls to parameterized queries or ORM methods in a subsequent sprint. - Severity: P2 (pre-existing, not a regression)
DR-2: ViewConfigPanel.vue container component not deeply reviewed (P2, design-lens, confidence MEDIUM)
- File:
src/agentkit/server/frontend/src/components/bitable/ViewConfigPanel.vue - Description: The container component that composes GroupingEditor + ConditionalFormatEditor was not deeply reviewed in this pass. Based on architecture, it is a composition layer (both child components were deeply reviewed and PASS). E2E specs
bitable-view.spec.tsandbitable-grouping.spec.tscover end-to-end behavior. - Suggested fix: Quick review during PR review to confirm props passthrough and event emit wiring.
- Severity: P2 (mitigated by child component review + e2e coverage)
DR-3: Design token lacks independent unit test (P3, test-coverage, confidence LOW)
- File:
src/agentkit/server/frontend/src/styles/bitable-tokens.css - Description: CSS tokens are validated via typecheck + e2e visual regression, but lack an independent test asserting key tokens (
--bitable-color-*,--bitable-cf-*,--bitable-drawer-width) are defined in:root. - Suggested fix: Optional — add a simple test that parses the CSS file and asserts token presence.
- Severity: P3 (optional polish)
DR-4: TOCTOU race in service.delete_view (P2, NEW in final pass, confidence HIGH)
- File:
src/agentkit/bitable/service.pydelete_viewmethod - Description: The
list_viewscount check and thedelete_viewcall are not atomic. Two concurrent DELETE /views requests for the last two views on a table can both pass thelen(siblings) <= 1guard before either commits, leaving the table with zero views and breaking the invariant the 409 protects. - Suggested fix: Move the siblings-count guard into the repository's
delete_viewas an atomic conditional delete (DELETE ... WHERE table_id=:t AND (SELECT COUNT(*) FROM views WHERE table_id=:t) > 1 RETURNING id), or wrap the check+delete in aSELECT ... FOR UPDATEtransaction. - Severity: P2 (real but narrow — single-user v1 target has low concurrency; concurrent agent + UI deletes could trip it)
- Reviewers: adversarial, security, reliability
DR-5: _update_field silently drops type changes (P2, NEW in final pass, confidence HIGH)
- File:
src/agentkit/tools/bitable_tool.py_update_fieldmethod - Description: The tool forwards
typeasfield_typein the payload, butUpdateFieldRequest(Pydantic v2, extra=ignore) silently drops the field. Agents callingupdate_fieldwith atypeargument receive 200/204 (success) while the backend never persisted the type change. This is an agent-native contract gap: the tool reports success for an unsupported operation, corrupting the agent's world model and potentially triggering infinite retry loops. - Suggested fix: Either (a) extend
UpdateFieldRequestto acceptfield_typeand forward it to the repository PATCH, or (b) drop thefield_typeforwarding from the tool and return 400/422 whentypeis set but unsupported, so the agent does not silently retry. - Severity: P2 (agent-native contract gap — false success on unsupported operation)
- Reviewers: agent-native, api-contract, correctness
Context
- All 6 Implementation Units (U1-U6) verified PASS against plan requirements
- All 11 KTDs verified PASS
- All Open Questions resolved (WCAG bold default, empty states, drawer loading/error/404, save button loading, vxe-pc-ui dependency, last-view protection)
- 0 P0/P1 findings — no blockers for merge
- Final pre-merge review (2026-07-04) surfaced 2 new P2 (DR-4, DR-5); both are downstream-resolver manual fixes, neither blocks merge
- Solutions documented in
docs/solutions/architecture-patterns/bitable-agent-tool-parity-patterns.md