Conversation
The jsdom vitest suite grew to ~2600 tests and intermittently failed the spa-build job on the 2-core runner with no assertion - a worker OOM-killed mid-run (it passes cleanly on beefier local machines). Three changes: - vite.config.ts: bound the fork pool (maxForks 2) and give each worker a 4GB heap via poolOptions.forks.execArgv, so GC has room before OOM; add retry: 1 so a single genuinely-flaky test retries instead of failing the gate. - vitest.setup.ts: stub HTMLCanvasElement.getContext (2d) + toDataURL, which jsdom does not implement - removes the "Not implemented: getContext" log spam and keeps canvas-touching components (charts, previews) stable in tests. - ci.yml: set NODE_OPTIONS max-old-space-size for the main vitest process and retry the step once as a belt-and-suspenders so a rare transient worker death never blocks a release. Full suite stays green locally (321 files, 2610 tests) with the getContext warnings gone.
…essage (#174) The answer_decision handler routed a generic reply to the asking agent AND each grant handler (execution, delegation) routed its own, so a gated decision sent the agent two messages. Make all three _apply_*_grant functions return whether they routed; the caller sends the generic reply only when none did. Also make the delegation reply honest: only claim the task was assigned when complete_delegation actually succeeded. A failed assignment now tells the agent to retry instead of reporting success.
…ec message) - Delegation: write the delegate grant only after complete_delegation succeeds, so a failed assignment no longer leaks a grant that would let a later delegation skip approval. Test asserts no live grant on failure. - Execution gate: track whether the grant write persisted; only say 'you may retry' when it did, otherwise tell the agent the grant save failed and to retry (a retry would re-prompt, so the old message was misleading). - Document that _apply_app_grant intentionally always returns False (app grants send no agent reply; the caller routes the generic answer).
fix(governance): dedupe decision-answer routing + honest delegation message (#174)
The built-in Weather app fetches the open-meteo geocoding (city search) and forecast APIs directly from the browser, but the global CSP connect-src was 'self' ws: wss: data: only. default-src 'self' therefore blocked every lookup, and searchLocations swallows the error into an empty result, so the search field looked completely dead with no visible error. Add both open-meteo origins to connect-src. Regression test asserts they are present.
…1603) restoreActiveTheme re-applied the active theme's declared default wallpaper on every boot, overriding a wallpaper the user had picked while on that theme (the wallpaper is persisted separately and restored by useSessionPersistence). That is the 'wallpaper always resets' report. Stop applying the theme default on restore: the persisted pick is authoritative, and a theme's default is already persisted when the theme is selected, so it still survives restore. Drop the now-dead restore branch of applyThemeDefaultWallpaper and update the test to assert the persisted pick wins.
fix(weather): allow open-meteo origins in CSP connect-src (#1668)
…r-theme fix(desktop): user's wallpaper pick wins over theme default on login (#1603)
…ped (Apple client slice 1) Device registration + management endpoints over DeviceStore, scoped to the session user (register returns the scoped_token once; list/patch/delete hide it). Two adjustments beyond the plan draft, both deliberate: - Devices are strictly personal, so _owned_or_404 has NO admin bypass (a device holds a per-device scoped token + its owner's sensor grants; even an admin manages only its own devices here). This matches the isolation the route test asserts. - The lifespan-bypassing test client fixture now inits device_store (it hand -inits lifespan-owned stores; device_store was the first route-tested one missing from that list).
…ple client slice 1)
… sandbox, touch debounce)
Real findings folded:
- Cap devices per user (50) and bound display_name/push_token lengths so a
looping client cannot issue unbounded scoped tokens or exhaust storage.
- require_device debounces the last_seen write (>=60s) so auth is not a
per-request DB write; Bearer scheme match is now case-insensitive (RFC 6750).
- apns_sender_from_env honours TAOS_APNS_SANDBOX (dev gateway) and warns when
the .p8 signing key is group/world readable.
Deliberately NOT changed: the flagged missing JWT 'exp' is a false positive.
Apple provider tokens carry iss+iat and self-expire on the 1h iat window; exp
is not in Apple's spec and real APNs clients omit it. Deferred (safe/consistent
as-is): _row column ordering (SELECT built from the same constant), {error}
response shape (matches decisions.py), get() token exposure (current callers
pop it correctly), and 410-vs-5xx send distinction (send() is unused scaffolding
until the pull slice).
Docs-Reviewed: Slice 1 device routes are internal server-foundation with no
cross-agent coordination surface yet; documented in the design spec, so no
agent-coordination.md change is warranted.
feat(devices): taOS Apple client — Slice 1 server foundation (device registry + scoped tokens + APNs)
…, Apple client foundation)
release: 1.0.0-beta.32
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
👋 Thanks for the PR! This one targets See CONTRIBUTING.md for the branch model. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (26)
📝 WalkthroughWalkthroughThis PR bundles CI/Vitest reliability fixes, a desktop wallpaper restore correction, a new device registry/auth/APNs backend for native iOS/watchOS clients, a decisions governance routing fix to avoid duplicate messages and report retry status, a CSP fix allowing Open-Meteo origins, and a version bump to 1.0.0-beta.32 with changelog. ChangesDesktop Vitest/CI reliability
Desktop wallpaper restore fix
Native iOS/watchOS device backend
Decisions governance routing fix
Weather CSP allowlist fix
Version bump and changelog
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant DevicesRouter
participant DeviceStore
participant HttpApnsSender
Client->>DevicesRouter: POST /api/devices/register
DevicesRouter->>DeviceStore: register(user_id, platform, push_token)
DeviceStore-->>DevicesRouter: device with scoped_token
DevicesRouter-->>Client: device record
Client->>DevicesRouter: request with Bearer scoped_token
DevicesRouter->>DeviceStore: get_by_token(token)
DeviceStore-->>DevicesRouter: device
DevicesRouter->>HttpApnsSender: send(push_token, payload)
HttpApnsSender-->>DevicesRouter: success/failure
sequenceDiagram
participant Agent
participant DecisionsRoute
participant GrantHandler
Agent->>DecisionsRoute: answer decision
DecisionsRoute->>GrantHandler: apply app/execution/delegation grant
GrantHandler-->>DecisionsRoute: routed=True/False
alt no handler routed
DecisionsRoute-->>Agent: generic answer message
else handler routed
DecisionsRoute-->>Agent: kind-specific message (denied/approved/retry)
end
Possibly related PRs
✨ 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 |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| device = await store.get_by_token(token) | ||
| if device is None: | ||
| raise HTTPException(status_code=401, detail="invalid device token") | ||
| if time.time() - device["last_seen"] > _TOUCH_INTERVAL_S: |
There was a problem hiding this comment.
CRITICAL: device["last_seen"] is text, not int — this will raise TypeError on every authenticated request.
DeviceStore.touch() writes strftime('%s','now'), and aiosqlite returns TEXT affinity for that expression (the column is declared INTEGER but strftime always yields a string at the SQLite boundary). So device["last_seen"] is a str, and time.time() - "1234567890" raises TypeError: unsupported operand type(s) for -: 'float' and 'str'. That 500s every authenticated device route after the first deploy of any device that ever authenticated.
| if time.time() - device["last_seen"] > _TOUCH_INTERVAL_S: | |
| if time.time() - int(device["last_seen"]) > _TOUCH_INTERVAL_S: |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| body: RegisterIn, request: Request, user: CurrentUser = Depends(current_user) | ||
| ): | ||
| store = request.app.state.device_store | ||
| if len(await store.list_for_user(user.user_id)) >= _MAX_DEVICES_PER_USER: |
There was a problem hiding this comment.
WARNING: TOCTOU on the per-user device cap — two concurrent /api/devices/register calls can both see len(...) == 49 and both insert, overshooting _MAX_DEVICES_PER_USER.
The cap is meant to bound token issuance + storage; running list_for_user followed by register outside a transaction lets a parallel request slip past. SQLite's default isolation won't save you here because the reads/writes are in separate awaits. Wrap the check + insert in a single BEGIN IMMEDIATE transaction (or use a unique partial index on (user_id) WHERE revoked = 0 with a row-count guard via a server-side INSERT ... SELECT WHERE (SELECT COUNT(*) ...) < N). Otherwise an attacker can pin the server by spamming register and blowing past the intended 50.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| callers treat the device as unreachable rather than assuming delivery.""" | ||
|
|
||
| async def send(self, push_token: str, payload: dict, *, topic: str | None = None) -> bool: | ||
| logger.info("APNs not configured; dropping push to %s", push_token[:8]) |
There was a problem hiding this comment.
WARNING: push_token[:8] raises IndexError if a caller ever sends an empty push_token.
RegisterIn.push_token defaults to "" and _MAX_PUSH_TOKEN = 4096 only caps length, not emptiness. NullApnsSender.send will crash on a 0-byte token. Guard the slice (e.g. push_token[:8] if push_token else "<empty>") or skip the log fragment for short tokens. This is the documented "log the intent" path used in every test of NullApnsSender, so it ships frequently.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| except httpx.HTTPError: | ||
| logger.warning("APNs send failed for %s", push_token[:8], exc_info=True) | ||
| return False | ||
| return resp.status_code == 200 |
There was a problem hiding this comment.
WARNING: resp.status_code == 200 treats every APNs non-200 as a transient failure. A 410 Gone (token permanently unregistered, device uninstalled) is being returned to callers as "send failed, try again later" — the device entry in devices will never be reaped and push will keep retrying a dead token. APNs-defined semantics:
200: delivered410: token is permanently invalid, markrevoked = 1(or setpush_token = '') and stop retrying403: provider token / key issue, misconfiguration — log loudly429: rate limit, retry withRetry-After5xx: transient, retry
Right now none of those distinctions reach DeviceStore, so the device registry will accumulate dead rows. Even though send() is "unused scaffolding until the pull slice", the response-shape contract is fixed here and the slice-2 caller will inherit a broken revocation signal.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| ) -> dict: | ||
| assert self._db is not None | ||
| device_id = uuid.uuid4().hex | ||
| scoped_token = DEVICE_TOKEN_PREFIX + secrets.token_urlsafe(32) |
There was a problem hiding this comment.
WARNING: scoped_token is stored in plaintext at rest. If devices.db is exfiltrated (backup, fs snapshot, repo debug tarball), every active device token is usable — they are the only credential a mobile client carries.
Storing hashlib.sha256(token_bytes).hexdigest() and returning the raw token only once in register() matches best practice for bearer tokens (and the precedent set by GitHub PATs, OAuth refresh tokens, etc.). Worth doing before the actual mobile app ships, because the first thing that needs to migrate to a hashed store later is all live device rows — a one-shot opportunity being missed here.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| return {"items": await store.list_for_user(user.user_id)} | ||
|
|
||
|
|
||
| async def _owned_or_404(store, device_id: str, user: CurrentUser): |
There was a problem hiding this comment.
SUGGESTION: _owned_or_404 calls store.get(...) which uses _FULL_COLS (loads scoped_token into memory) only to check ownership. The secret never needs to leave the store for a 404 check.
A store.get_owned(device_id, user_id) -> bool doing SELECT 1 FROM devices WHERE device_id = ? AND user_id = ? AND revoked = 0 keeps the secret off the request path entirely and saves a column-list parse. Defensive in depth: a future bug that forgets to pop("scoped_token") after a get() here can't leak the token through the 404-not-found branch.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 6 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (26 files)
Notes
Fix these issues in Kilo Cloud Reviewed by minimax-m3 · Input: 69.2K · Output: 5.7K · Cached: 398.8K |
1 participant
Promotion of dev to master for the v1.0.0-beta.32 release.
Batches since beta.31:
Tag + release will be cut on the resulting master SHA.
Summary by CodeRabbit
New Features
Bug Fixes
Chores