fix: V1 release blockers — 7 P1 security + correctness fixes - #46
Merged
Conversation
…s-48k) After keyring write succeeds, clean up any leftover fernet.key file on disk. Logs warning if removal fails but doesn't block the operation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…-2fd) Derive keyring username from SHA-256 hash of resolved home_dir path so two worthless installs on the same machine get unique keyring entries. Legacy fallback in read_fernet_key for existing installs. delete cleans both new and legacy entries. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add migrate_file_to_keyring() that opportunistically promotes file-based Fernet keys to the OS keyring. Called from ensure_home after successful key read. Never raises — all failures swallowed to debug log. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…n (worthless-3sd) SR-01/SR-02 compliance: _read_fernet_key() returns bytearray, ProxySettings.fernet_key is bytearray, ShardRepository stores bytearray with close() that zeros it. Lifespan shutdown zeros all reachable key material. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…t overwrite (worthless-ks6) The read-check-update cycle on the sliding window was not atomic across await boundaries. Concurrent requests could observe stale window state, causing both undercounting (burst bypass) and potential overcounting (false-positive denials). Per-key asyncio.Lock serializes the critical section. Stale locks are cleaned alongside stale windows. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…thless-m58) Reverse write order in _enroll_single() to match _lock_keys pattern: DB commit first (atomic point), shard_a file second. On failure, compensate by cleaning up whichever artifact was written. Prevents orphan shard_a files when DB write fails. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ess-kyc) Add StreamingUsageCollector that extracts token usage from SSE chunks incrementally without buffering entire responses. Replace collected_chunks list with the collector in the streaming path. Remove BodySizeLimitMiddleware and max_request_bytes — we're a transparent pipe, upstream providers enforce their own limits (Anthropic: 32MB, Bedrock: 20MB). Adding our own creates friction for zero benefit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Tests written from the attacker's POV — passing means the attack fails. Covers: stale key exfiltration, cross-install keyring theft, memory dump key extraction, rate limit burst bypass, enrollment crash exploitation, and streaming OOM attack. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…nsation 1. Streaming metering fails closed: when usage extraction returns None, record a pessimistic 10k token estimate so spend caps still enforce. (Brutus finding: silent None bypasses spend cap) 2. _enroll_single compensation uses sync sqlite3 instead of nested asyncio.run, and only deletes the specific enrollment instead of CASCADE-deleting all enrollments for the alias. (Brutus + Karen findings: event loop collision, over-broad delete) 3. Adversarial lifespan test now exercises actual _lifespan shutdown path instead of manually zeroing. (Jenny finding: test didn't prove shutdown code runs) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add _get_fernet() helper that raises RuntimeError if called after close(). Fixes pyright reportOptionalMemberAccess errors. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughEnsure existing Fernet keys migrate into per-home keyring entries; make Fernet keys mutable bytearrays and zero them on shutdown; add two-phase enroll with compensating cleanup; introduce an incremental SSE StreamingUsageCollector; add per-(alias,client_ip) rate-limit locks; remove Content-Length body-size middleware. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Bootstrap as Bootstrap
participant Keystore as Keystore
participant OSKeyring as OS Keyring
participant FileSystem as File System
User->>Bootstrap: Start application
Bootstrap->>Keystore: ensure_home()
Keystore->>Keystore: read_fernet_key(home_dir)
alt Key found
Keystore-->>Bootstrap: bytearray key
Bootstrap->>Keystore: migrate_file_to_keyring(home_dir)
alt fernet.key file exists
Keystore->>FileSystem: Read plaintext key file
FileSystem-->>Keystore: Key bytes
Keystore->>OSKeyring: Store under namespaced username
OSKeyring-->>Keystore: Stored
Keystore->>FileSystem: Delete fernet.key
FileSystem-->>Keystore: Deleted
else File missing
Keystore-->>Bootstrap: Migration skipped
end
else KEY_NOT_FOUND
Keystore-->>Bootstrap: KEY_NOT_FOUND
Bootstrap->>Keystore: Generate & store new Fernet key
end
sequenceDiagram
actor Client
participant EnrollCmd as Enroll Command
participant Repository as ShardRepository
participant DB as SQLite DB
participant FS as File System
Client->>EnrollCmd: Enroll request
EnrollCmd->>Repository: _enroll_single()
Repository->>DB: async store_enrolled(...)
alt DB commit succeeds
DB-->>Repository: Committed
Repository->>FS: write shard_a file
alt File write succeeds
FS-->>Repository: Written
Repository-->>EnrollCmd: Success
else File write fails
Repository->>DB: sync DELETE enrollment row where key_alias & env_path IS NULL
DB-->>Repository: Cleaned
Repository-->>EnrollCmd: Error (compensated)
end
else DB write fails
DB-->>Repository: Error
Repository-->>EnrollCmd: Error (no file created)
end
sequenceDiagram
participant Stream as Streaming Response
participant Collector as StreamingUsageCollector
participant App as Proxy App
Stream->>Collector: feed(chunk)
Collector->>Collector: Parse SSE incrementally (partial line state)
alt chunk contains usage
Collector->>Collector: Extract token totals & model
else no usage yet
Collector->>Collector: Maintain bounded partial state
end
App->>Collector: result()
alt Usage found
Collector-->>App: UsageInfo(tokens, model)
App->>App: record_spend(actual usage)
else No usage
Collector-->>App: None
App->>App: skip spend recording (log warning)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 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 |
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Cap _partial_line at 64KB to prevent unbounded growth from malicious upstream without newlines. Add _locks dict assertions to rate limiter cleanup tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…hans 1. Remove legacy keyring fallback entirely — no users exist, no legacy to support. Read and delete now use namespaced username only. 2. Orphan shard_a self-healing: detect file-exists-with-no-DB-row before enrollment and clean up automatically. 3. Compensation DELETE includes var_name for precision. 4. Move sqlite3 import to top of file. 5. Pessimistic stream estimate removed — zero friction, log warning only when usage extraction fails. 6. Add 4 edge-case tests for StreamingUsageCollector (split chunks, malformed JSON, empty stream, partial_line cap). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tion _read_fernet_key() now passes home_dir from WORTHLESS_HOME env var to read_fernet_key(), so the keyring username hash matches the home_dir used at enrollment time. Without this, custom WORTHLESS_HOME (e.g. in tests or Docker) would hash to a different keyring entry and fail to find the key. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Changed model from claude-3-5-haiku-latest to claude-3-haiku-20240307. The old model returned 404 — not a proxy bug, just an inaccessible model name for the test API key. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
ProxySettings.fernet_key is now bytearray, not str. Test fixtures that passed fernet_key.decode() (str) worked on macOS (keyring available, test bypassed) but failed on headless Ubuntu CI where test_contract.py actually starts the proxy and hits bytearray(str) TypeError. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
addopts now includes -m 'not live and not docker' so plain `uv run pytest` runs everything except tests needing API keys or Docker. Use `uv run pytest -m live` or `-m docker` explicitly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The fixture used _free_port() which can race with cleanup on reruns — the old container's port isn't released before the new one binds. Use -p 127.0.0.1::8787 (Docker-assigned port) and discover via docker port after start. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
src/worthless/proxy/rules.py (1)
334-336: Consider moving DB call outside the lock.The
await self._load_limit(alias)performs a database query while holding the per-key lock. If the DB is slow, this blocks all concurrent requests for this(alias, client_ip)pair. Since limits are per-alias (not per-key), the load could be done before acquiring the per-key lock, or use a separate per-alias lock for limit loading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worthless/proxy/rules.py` around lines 334 - 336, The per-key lock is held while awaiting self._load_limit(alias), which does DB I/O and can block other requests for the same (alias, client_ip); change the logic so the DB load happens outside the per-key lock or use a dedicated per-alias lock: first check if alias not in self._limits and self.db_path is not None then perform await self._load_limit(alias) (or acquire a per-alias asyncio lock keyed by alias and load into self._limits under that lock), and only after the limit is populated, acquire the existing per-key lock to read self._limits.get(alias, self.default_rps) and proceed. Ensure you reference and update self._limits and default_rps under the appropriate lock to avoid races.src/worthless/cli/commands/lock.py (1)
310-319: Potential TOCTOU race in orphan cleanup.Between checking
shard_a_path.exists()andshard_a_path.unlink(), another process could remove or modify the file. While this is a best-effort cleanup of orphans from prior failed enrollments, consider usingmissing_ok=Trueto avoid a potentialFileNotFoundError.🛡️ Defensive fix for race condition
if shard_a_path.exists(): conn = sqlite3.connect(str(home.db_path)) try: row = conn.execute("SELECT 1 FROM shards WHERE key_alias = ?", (alias,)).fetchone() finally: conn.close() if row is None: - shard_a_path.unlink() + shard_a_path.unlink(missing_ok=True)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worthless/cli/commands/lock.py` around lines 310 - 319, The orphan shard cleanup currently checks shard_a_path.exists() and later calls shard_a_path.unlink(), which can raise FileNotFoundError due to a TOCTOU race; update the unlink call to be defensive by using shard_a_path.unlink(missing_ok=True) (or catch FileNotFoundError around shard_a_path.unlink()) while keeping the existing DB check logic (refer to shard_a_path.exists(), shard_a_path.unlink() and the surrounding try/finally block that queries the shards table) so the cleanup becomes no-op if the file was removed by another process.tests/test_streaming_metering.py (1)
1-5: Outdated docstring — tests are no longer RED phase.The module docstring states "RED tests: these should FAIL because StreamingUsageCollector does not exist yet." However, based on the AI summary and test structure,
StreamingUsageCollectorhas been implemented. Consider updating the docstring to reflect the current state.📝 Update docstring
-"""Streaming metering tests — StreamingUsageCollector must extract usage -from SSE chunks without buffering entire responses. - -RED tests: these should FAIL because StreamingUsageCollector does not exist yet. -""" +"""Streaming metering tests — StreamingUsageCollector must extract usage +from SSE chunks without buffering entire responses. +"""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_streaming_metering.py` around lines 1 - 5, Update the module docstring in tests/test_streaming_metering.py to remove the outdated "RED tests" statement and reflect that StreamingUsageCollector has been implemented; specifically, replace or reword the lines referencing "RED tests: these should FAIL because StreamingUsageCollector does not exist yet" to state that the tests verify StreamingUsageCollector correctly extracts usage from SSE chunks without buffering entire responses, and mention current expected pass status rather than failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 7: The README contains a Markdown badge with an empty link
"[]()" which
triggers MD042; update the badge by either providing a valid URL to your
CI/status page (replace the empty link parentheses with the CI badge URL) or
remove the badge entirely until you have a proper link; look for the literal
badge string
"[]()" to locate
and modify the line.
In `@src/worthless/proxy/app.py`:
- Around line 398-416: The current _record_metering coroutine treats a None from
usage_collector.result() as a no-op and only logs a warning, which allows
streamed requests to bypass spend caps; change this to a fail-closed behavior:
inside async def _record_metering() check if usage_collector.result() is None
and in that branch either (1) compute a conservative fallback (e.g., a
configurable fallback_token_count and fallback_model string) and call
record_spend(settings.db_path, alias, fallback_token_count, fallback_model,
encrypted.provider) before returning, or (2) reject the stream by raising an
error/recording an authorization failure so the request is blocked; update
logging to include that a fallback charge or reject was applied and ensure
usage_collector.result(), record_spend(), and logger.warning calls are used to
implement this change.
- Around line 173-178: The shutdown path currently calls await client.aclose(),
await db.close(), and repo.close() before zeroing settings.fernet_key, so if
client.aclose() or db.close() raises the key wipe is skipped; move the
zeroization into an outer finally block that always executes (surround the
shutdown calls with try/finally), ensure settings.fernet_key is zeroed in the
finally even if repo.close()/client.aclose()/db.close() fail, and keep any
existing close/await calls in the try so their exceptions still propagate after
the key is securely zeroed.
In `@src/worthless/proxy/metering.py`:
- Around line 195-208: The result() method can miss an unterminated final SSE
line left in _partial_line by feed(); before computing usage in result() (for
both providers) ensure any buffered tail is parsed—if self._partial_line is
non-empty, call the same parsing path used by feed() (e.g., invoke the internal
parser or _parse_event on the remaining _partial_line, or emulate feed() by
appending a newline and processing it) so _found_usage and token counters
(_input_tokens, _output_tokens, _total_tokens) are updated before returning
UsageInfo from result().
In `@src/worthless/proxy/rules.py`:
- Around line 322-326: The per-key lock creation has a TOCTOU race: replace the
non-atomic check-then-create pattern around self._locks with an atomic creation
so only one Lock is created per key; specifically, use an atomic operation (e.g.
dict.setdefault or dict.get combined with assignment in a thread-safe way) to
ensure self._locks[key] is created exactly once instead of using "if key not in
self._locks: self._locks[key] = asyncio.Lock()"; update the code paths that
reference self._locks and asyncio.Lock() so they obtain the lock via this atomic
creation (keep using the same lock instance for subsequent awaits).
In `@tests/test_cli_lock.py`:
- Around line 564-567: The test is asserting on shard keys via repo.list_keys()
but should assert that there are no DB enrollment rows; replace the call to
repo.list_keys() with the enrollment-specific method (e.g.,
repo.list_enrollments() or the project’s equivalent) and assert the returned
enrollments list is empty, updating the failure message to mention "enrollment
row(s) not cleaned up" and referencing the same repo variable to locate the
change.
---
Nitpick comments:
In `@src/worthless/cli/commands/lock.py`:
- Around line 310-319: The orphan shard cleanup currently checks
shard_a_path.exists() and later calls shard_a_path.unlink(), which can raise
FileNotFoundError due to a TOCTOU race; update the unlink call to be defensive
by using shard_a_path.unlink(missing_ok=True) (or catch FileNotFoundError around
shard_a_path.unlink()) while keeping the existing DB check logic (refer to
shard_a_path.exists(), shard_a_path.unlink() and the surrounding try/finally
block that queries the shards table) so the cleanup becomes no-op if the file
was removed by another process.
In `@src/worthless/proxy/rules.py`:
- Around line 334-336: The per-key lock is held while awaiting
self._load_limit(alias), which does DB I/O and can block other requests for the
same (alias, client_ip); change the logic so the DB load happens outside the
per-key lock or use a dedicated per-alias lock: first check if alias not in
self._limits and self.db_path is not None then perform await
self._load_limit(alias) (or acquire a per-alias asyncio lock keyed by alias and
load into self._limits under that lock), and only after the limit is populated,
acquire the existing per-key lock to read self._limits.get(alias,
self.default_rps) and proceed. Ensure you reference and update self._limits and
default_rps under the appropriate lock to avoid races.
In `@tests/test_streaming_metering.py`:
- Around line 1-5: Update the module docstring in
tests/test_streaming_metering.py to remove the outdated "RED tests" statement
and reflect that StreamingUsageCollector has been implemented; specifically,
replace or reword the lines referencing "RED tests: these should FAIL because
StreamingUsageCollector does not exist yet" to state that the tests verify
StreamingUsageCollector correctly extracts usage from SSE chunks without
buffering entire responses, and mention current expected pass status rather than
failure.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 03a50359-0dad-4168-8fd1-c078a2fc4ab8
📒 Files selected for processing (25)
README.mdpyproject.tomlsrc/worthless/cli/bootstrap.pysrc/worthless/cli/commands/lock.pysrc/worthless/cli/keystore.pysrc/worthless/proxy/app.pysrc/worthless/proxy/config.pysrc/worthless/proxy/metering.pysrc/worthless/proxy/middleware.pysrc/worthless/proxy/rules.pysrc/worthless/storage/repository.pytests/test_adversarial.pytests/test_cli_lock.pytests/test_config.pytests/test_contract.pytests/test_docker_e2e.pytests/test_e2e_live.pytests/test_error_metering_and_hardening.pytests/test_fernet_bytearray.pytests/test_keystore.pytests/test_proxy.pytests/test_proxy_hardening.pytests/test_proxy_keyring.pytests/test_rules.pytests/test_streaming_metering.py
💤 Files with no reviewable changes (1)
- src/worthless/proxy/middleware.py
Comment on lines
398
to
+416
| async def _record_metering(): | ||
| await _do_record_spend(b"".join(collected_chunks)) | ||
| usage = usage_collector.result() | ||
| if usage is not None: | ||
| await record_spend( | ||
| settings.db_path, | ||
| alias, | ||
| usage.total_tokens, | ||
| usage.model, | ||
| encrypted.provider, | ||
| ) | ||
| else: | ||
| # Zero friction: if we can't extract usage (provider | ||
| # changed SSE format, etc.), log a warning but don't | ||
| # penalize the user with phantom spend. | ||
| logger.warning( | ||
| "Could not extract usage from streaming response " | ||
| "for alias=%s; spend not recorded", | ||
| alias, | ||
| ) |
There was a problem hiding this comment.
Don't fail open when streamed usage extraction misses.
If usage_collector.result() returns None because the provider changes its SSE shape or the stream terminates unexpectedly, this path records nothing. That makes spend caps and token budgets effectively free for streamed requests. Keep the fail-closed behavior here by charging a conservative fallback or rejecting the stream.
Proposed fix
async def _record_metering():
- usage = usage_collector.result()
- if usage is not None:
- await record_spend(
- settings.db_path,
- alias,
- usage.total_tokens,
- usage.model,
- encrypted.provider,
- )
- else:
- # Zero friction: if we can't extract usage (provider
- # changed SSE format, etc.), log a warning but don't
- # penalize the user with phantom spend.
- logger.warning(
- "Could not extract usage from streaming response "
- "for alias=%s; spend not recorded",
- alias,
- )
+ try:
+ usage = usage_collector.result()
+ if usage is None:
+ fallback_tokens = 10_000 # or a shared fail-closed constant
+ logger.warning(
+ "Could not extract usage from streaming response for alias=%s; "
+ "recording fallback usage=%d",
+ alias,
+ fallback_tokens,
+ )
+ await record_spend(
+ settings.db_path,
+ alias,
+ fallback_tokens,
+ None,
+ encrypted.provider,
+ )
+ return
+
+ await record_spend(
+ settings.db_path,
+ alias,
+ usage.total_tokens,
+ usage.model,
+ encrypted.provider,
+ )
+ except Exception:
+ logger.warning("Failed to record spend for alias=%s", alias)📝 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.
Suggested change
| async def _record_metering(): | |
| await _do_record_spend(b"".join(collected_chunks)) | |
| usage = usage_collector.result() | |
| if usage is not None: | |
| await record_spend( | |
| settings.db_path, | |
| alias, | |
| usage.total_tokens, | |
| usage.model, | |
| encrypted.provider, | |
| ) | |
| else: | |
| # Zero friction: if we can't extract usage (provider | |
| # changed SSE format, etc.), log a warning but don't | |
| # penalize the user with phantom spend. | |
| logger.warning( | |
| "Could not extract usage from streaming response " | |
| "for alias=%s; spend not recorded", | |
| alias, | |
| ) | |
| async def _record_metering(): | |
| try: | |
| usage = usage_collector.result() | |
| if usage is None: | |
| fallback_tokens = 10_000 # or a shared fail-closed constant | |
| logger.warning( | |
| "Could not extract usage from streaming response for alias=%s; " | |
| "recording fallback usage=%d", | |
| alias, | |
| fallback_tokens, | |
| ) | |
| await record_spend( | |
| settings.db_path, | |
| alias, | |
| fallback_tokens, | |
| None, | |
| encrypted.provider, | |
| ) | |
| return | |
| await record_spend( | |
| settings.db_path, | |
| alias, | |
| usage.total_tokens, | |
| usage.model, | |
| encrypted.provider, | |
| ) | |
| except Exception: | |
| logger.warning("Failed to record spend for alias=%s", alias) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/worthless/proxy/app.py` around lines 398 - 416, The current
_record_metering coroutine treats a None from usage_collector.result() as a
no-op and only logs a warning, which allows streamed requests to bypass spend
caps; change this to a fail-closed behavior: inside async def _record_metering()
check if usage_collector.result() is None and in that branch either (1) compute
a conservative fallback (e.g., a configurable fallback_token_count and
fallback_model string) and call record_spend(settings.db_path, alias,
fallback_token_count, fallback_model, encrypted.provider) before returning, or
(2) reject the stream by raising an error/recording an authorization failure so
the request is blocked; update logging to include that a fallback charge or
reject was applied and ensure usage_collector.result(), record_spend(), and
logger.warning calls are used to implement this change.
Comment on lines
+322
to
+326
| # Per-key lock serializes the read-check-update cycle to prevent | ||
| # concurrent requests from observing stale window state (worthless-ks6). | ||
| if key not in self._locks: | ||
| self._locks[key] = asyncio.Lock() | ||
|
|
There was a problem hiding this comment.
Race condition in lazy lock creation.
There's a TOCTOU race between checking key not in self._locks and creating the lock. Two concurrent requests for the same key could both create locks, with one being discarded, allowing both to proceed without proper serialization.
🔒 Proposed fix using setdefault for atomic lock creation
- # Per-key lock serializes the read-check-update cycle to prevent
- # concurrent requests from observing stale window state (worthless-ks6).
- if key not in self._locks:
- self._locks[key] = asyncio.Lock()
-
- async with self._locks[key]:
+ # Per-key lock serializes the read-check-update cycle to prevent
+ # concurrent requests from observing stale window state (worthless-ks6).
+ lock = self._locks.setdefault(key, asyncio.Lock())
+
+ async with lock:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/worthless/proxy/rules.py` around lines 322 - 326, The per-key lock
creation has a TOCTOU race: replace the non-atomic check-then-create pattern
around self._locks with an atomic creation so only one Lock is created per key;
specifically, use an atomic operation (e.g. dict.setdefault or dict.get combined
with assignment in a thread-safe way) to ensure self._locks[key] is created
exactly once instead of using "if key not in self._locks: self._locks[key] =
asyncio.Lock()"; update the code paths that reference self._locks and
asyncio.Lock() so they obtain the lock via this atomic creation (keep using the
same lock instance for subsequent awaits).
Semgrep requires annotations on the same line as the flagged code. These are ephemeral bytes() conversions for Fernet/SQLite API calls — not stored key material. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| self._fernet_key_bytes = key_bytes # kept for HMAC-SHA256 decoy hashing | ||
| self._fernet_key_bytes = bytearray(fernet_key) # SR-01: mutable for zeroing | ||
| self._fernet: Fernet | None = Fernet( | ||
| bytes(self._fernet_key_bytes) |
| # nosemgrep: sr01-key-material-not-bytearray (ephemeral bytes for Fernet/sqlite I/O) | ||
| shard_b_enc = self._fernet.encrypt(bytes(shard.shard_b)) | ||
| shard_b_enc = self._get_fernet().encrypt( | ||
| bytes(shard.shard_b) |
| # nosemgrep: sr01-key-material-not-bytearray (ephemeral bytes for Fernet/sqlite I/O) | ||
| shard_b_enc = self._fernet.encrypt(bytes(shard.shard_b)) | ||
| shard_b_enc = self._get_fernet().encrypt( | ||
| bytes(shard.shard_b) |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/worthless/proxy/rules.py (1)
317-320:⚠️ Potential issue | 🟠 MajorDon't evict a per-key lock while another request may still be using it.
A request can acquire the old lock, suspend in
await self._load_limit(alias), and meanwhile another request can run_cleanup(), dropself._locks[key], create a new lock, and enter the same(alias, client_ip)window concurrently. That reopens the race this change is trying to close.🔒 Minimal safe fix
for k in stale_keys: del self._windows[k] - self._locks.pop(k, None) + # Do not evict the lock here. Another coroutine may already be + # holding or waiting on this key's lock while `evaluate()` is + # suspended in `_load_limit()`.Also applies to: 324-327, 333-336, 356-358
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worthless/proxy/rules.py` around lines 317 - 320, The cleanup code must not drop a per-key asyncio.Lock while another coroutine may still hold it; update the cleanup logic (the periodic block that calls self._cleanup(now) and inside _cleanup) to only remove entries from self._locks when the existing lock is not currently held (use lock.locked() to skip deletion if True) and avoid replacing/removing locks in other cleanup paths (the spots around the calls to self._cleanup, and the other cleanup locations handling self._locks referenced in _load_limit and related code). Concretely, before deleting self._locks[key] or creating a new lock for the same key, check the existing lock with if existing_lock.locked(): continue/skip removal so you never evict a lock that may be in-use by an awaiting request.src/worthless/proxy/app.py (1)
201-203:⚠️ Potential issue | 🟠 MajorKeep a proxy-side request-size guard.
Provider-side limits happen too late here because
proxy_request()still materializes the entire request body before routing. With the ingress cap removed, a client can force this process to allocate arbitrarily large payloads in the hot path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worthless/proxy/app.py` around lines 201 - 203, The proxy currently lets proxy_request() materialize the full incoming body because the ingress cap was removed; add an early request-size guard middleware (registered before any handler code) to enforce a hard maximum (e.g., MAX_PROXY_BODY_BYTES) so large uploads are rejected before proxy_request() reads the body. Implement a lightweight middleware (e.g., subclass BaseHTTPMiddleware or use app.add_middleware) that checks Content-Length and/or streams the body up to the limit and returns 413 if exceeded, and reference/replace the existing app.add_middleware(CORSMiddleware, ...) registration to ensure the size-guard runs prior to proxy_request().
♻️ Duplicate comments (5)
src/worthless/proxy/metering.py (1)
195-208:⚠️ Potential issue | 🟠 MajorFlush the buffered tail before computing the final usage.
feed()intentionally preserves an unterminated tail in_partial_line, butresult()never parses it. If the upstream closes right after the lastdata:line, usage is lost and the stream is under-metered.🧩 Suggested fix
def result(self) -> UsageInfo | None: """Return extracted usage after stream ends.""" + if self._partial_line: + tail = self._partial_line.strip() + self._partial_line = "" + if tail.startswith("event: "): + self._pending_event = tail[7:] + elif tail.startswith("data: "): + payload = tail[6:] + if payload != "[DONE]": + self._parse_data(payload) + if self.provider == "openai": if self._total_tokens is not None: return UsageInfo(total_tokens=self._total_tokens, model=self._model)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worthless/proxy/metering.py` around lines 195 - 208, The result() method fails to process any remaining buffered partial line stored in _partial_line before computing usage, causing undercounting when the stream ends; modify result() to first flush/parse the buffered tail by invoking the same parsing logic feed() uses (e.g., call the internal line-parsing routine or replicate its final-slice logic to handle _partial_line) so that _found_usage, _input_tokens, _output_tokens (and for openai, _total_tokens) reflect the last unterminated data: ensure you update/emit usage from the parsed tail before returning UsageInfo in result(), referencing result(), feed(), _partial_line, _parse_line (or the internal parsing code), _found_usage, _input_tokens, _output_tokens, and _total_tokens to locate and fix the logic.README.md (1)
7-7:⚠️ Potential issue | 🟡 MinorFix the empty Tests badge link.
This still trips MD042 and leaves the badge with a dead target.
🔗 Suggested fix
-[]() +[](https://github.com/shacharm2/worthless/actions)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 7, The README's Tests badge has an empty link target ("[]()") which triggers MD042; update the badge line by replacing the empty parentheses with a valid CI/status badge URL (or remove the link wrapper so the image is not a link) to ensure the badge points to a real resource — locate the Tests badge Markdown line in README.md and change the link target accordingly.src/worthless/proxy/app.py (2)
173-178:⚠️ Potential issue | 🟠 MajorMake shutdown zeroization unconditional.
If
client.aclose()ordb.close()raises, the Fernet key wipe never runs and the key stays live in memory. Put cleanup behind nestedfinallyblocks so zeroization always executes.Suggested fix
- await client.aclose() - await db.close() - repo.close() - # Zero the settings key material (SR-02) - for i in range(len(settings.fernet_key)): - settings.fernet_key[i] = 0 + try: + await client.aclose() + finally: + try: + await db.close() + finally: + try: + repo.close() + finally: + # Zero the settings key material (SR-02) + for i in range(len(settings.fernet_key)): + settings.fernet_key[i] = 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/worthless/proxy/app.py` around lines 173 - 178, The shutdown sequence currently calls await client.aclose(), await db.close(), and repo.close() sequentially so that if any of those raise the zeroization loop over settings.fernet_key never runs; wrap the teardown in nested try/finally blocks (or a single try with a finally) so that regardless of exceptions from client.aclose(), db.close(), or repo.close() the zeroization of settings.fernet_key always executes; locate the shutdown block containing client.aclose(), db.close(), repo.close(), and the for i in range(len(settings.fernet_key)) loop and move the zeroization into the finally that is guaranteed to run after attempting to close client/db/repo, optionally logging or re-raising errors after zeroization.
398-416:⚠️ Potential issue | 🔴 CriticalDon't let streaming metering fail open.
StreamingUsageCollector.result()returnsNonewhen extraction fails. In that branch this path records nothing, which makes streamed requests effectively free against spend caps and token budgets.Suggested fix
async def _record_metering(): - usage = usage_collector.result() - if usage is not None: - await record_spend( - settings.db_path, - alias, - usage.total_tokens, - usage.model, - encrypted.provider, - ) - else: - # Zero friction: if we can't extract usage (provider - # changed SSE format, etc.), log a warning but don't - # penalize the user with phantom spend. - logger.warning( - "Could not extract usage from streaming response " - "for alias=%s; spend not recorded", - alias, - ) + try: + usage = usage_collector.result() + if usage is None: + fallback_tokens = 10_000 + logger.warning( + "Could not extract usage from streaming response for alias=%s; " + "recording fallback usage=%d", + alias, + fallback_tokens, + ) + await record_spend( + settings.db_path, + alias, + fallback_tokens, + None, + encrypted.provider, + ) + return + + await record_spend( + settings.db_path, + alias, + usage.total_tokens, + usage.model, + encrypted.provider, + ) + except Exception: + logger.warning("Failed to record spend for alias=%s", alias)tests/test_cli_lock.py (1)
564-567:⚠️ Potential issue | 🟡 MinorAssert enrollments here, not shard keys.
list_keys()only proves the shard row is gone. This compensation path specifically deletes the enrollment row, so the test still passes if an orphanedenrollmentsrow is left behind.Suggested fix
- aliases = asyncio.run(repo.list_keys()) - assert aliases == [], f"DB enrollment row(s) not cleaned up after file failure: {aliases}" + enrollments = asyncio.run(repo.list_enrollments()) + assert enrollments == [], ( + f"DB enrollment row(s) not cleaned up after file failure: {enrollments}" + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cli_lock.py` around lines 564 - 567, The test currently asserts shard keys via repo.list_keys(), but the compensation path deletes the enrollment row specifically; replace or augment that check to assert enrollments are gone by calling the repository enrollment API: obtain the repo with _repo(home_dir) and call asyncio.run(repo.list_enrollments()) (or the repository method that returns enrollment rows) and assert it equals [] instead of relying on list_keys(); keep or adjust the shard key assertion if desired.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 33-35: The fenced code block showing "$ worthless wrap python
app.py" should be converted to a proper shell snippet or a real interactive
transcript: either change the fence language from "console" to "bash" and remove
the leading "$ " prompt so it becomes a plain shell snippet, or keep the
"console" fence and add the command output lines below the prompt to make it a
real console transcript; update the triple-backtick block containing the command
accordingly.
In `@src/worthless/cli/commands/lock.py`:
- Around line 321-357: The rollback logic deletes enrollments/shards
unconditionally when shard file creation fails, but store_enrolled() is INSERT
OR IGNORE so the DB write may have been a no-op for pre-existing aliases; update
the flow to avoid deleting rows you didn't create by either: (A) pre-check for
an existing alias before calling _enroll_async()/store_enrolled() and abort with
a clear error if alias exists, or (B) modify store_enrolled() to return which
row ids (or a boolean per-insert) it actually created and change the exception
handler to only delete those specific enrollment/shard rows (use those returned
ids instead of deleting WHERE key_alias = ?). Reference functions/variables:
store_enrolled(), _enroll_async(), shard_a_path, alias, home.db_path. Ensure
compensation only removes rows that this invocation created.
In `@src/worthless/cli/keystore.py`:
- Around line 64-80: store_fernet_key() currently logs "Fernet key stored in OS
keyring" but may fall back to _write_key_file(key, home_dir) on exception, and
migrate_file_to_keyring() treats any return as success; change the contract so
store_fernet_key() returns an explicit result (e.g., enum/boolean) indicating
which backend was used (keyring vs file) or raise on fallback, and update
migrate_file_to_keyring() to only report success/return True when the result
indicates the keyring backend was actually used; locate and update the functions
named store_fernet_key, migrate_file_to_keyring, and the fallback call to
_write_key_file and use keyring_available()/_fernet_file_path() as needed to
decide/report the correct outcome.
In `@src/worthless/proxy/config.py`:
- Around line 39-45: The try/except around converting fd_str to int and reading
from the inherited Fernet pipe (fd/fd_str, os.read) can skip os.close(fd) if
os.read() raises, leaking the descriptor; refactor so the read/close sequence
always closes the descriptor in a finally block: attempt to open/convert fd_str
to int, then in a try read raw = os.read(fd, 4096) and return
bytearray(raw.strip()) but ensure os.close(fd) is called from a finally clause,
and only suppress ValueError/OSError for the conversion/read while preserving
the unconditional close semantics before falling back to the keystore.
In `@src/worthless/proxy/metering.py`:
- Around line 166-193: In _parse_data, certain branches assume parsed["usage"]
and parsed["message"] are dicts and can raise when they are non-dict (e.g.,
{"usage":1} or {"message": []}); update the logic in the _parse_data method to
validate types before accessing dict methods: for provider == "openai" check
that parsed.get("usage") is a dict before using .get(...) and ignore/move on if
not; for provider == "anthropic" ensure parsed.get("message") is a dict when
handling "message_start" and that parsed.get("usage") is a dict when handling
"message_delta" (or coerce safely), so you silently skip malformed shapes and do
not raise while preserving
_pending_event/_found_usage/_total_tokens/_input_tokens/_output_tokens behavior.
In `@src/worthless/storage/repository.py`:
- Around line 98-102: close() currently zeros the key and clears _fernet but
does not prevent decoy-hash operations; add a shutdown guard (e.g. set
self._closed = True in close()) and update _compute_decoy_hash(),
set_decoy_hash(), and is_known_decoy() to check that guard (or raise
RuntimeError when self._closed is True) so they fail fast after shutdown instead
of operating on the zeroed key buffer; ensure the guard is initialized (False)
in the constructor and used consistently in those three methods.
In `@tests/test_e2e_live.py`:
- Line 63: The test currently hardcodes the deprecated model id
"claude-3-haiku-20240307"; replace that literal with the new recommended id
"claude-haiku-4-5-20251001" and update tests to read the model id from a single
source (e.g., an environment variable like TEST_ANTHROPIC_MODEL or a shared
constant TEST_MODEL_ID) so future rotations only require one change; locate the
string "claude-3-haiku-20240307" in tests/test_e2e_live.py and refactor the code
that sets the "model" field to use the env var or constant with a sensible
default of "claude-haiku-4-5-20251001".
---
Outside diff comments:
In `@src/worthless/proxy/app.py`:
- Around line 201-203: The proxy currently lets proxy_request() materialize the
full incoming body because the ingress cap was removed; add an early
request-size guard middleware (registered before any handler code) to enforce a
hard maximum (e.g., MAX_PROXY_BODY_BYTES) so large uploads are rejected before
proxy_request() reads the body. Implement a lightweight middleware (e.g.,
subclass BaseHTTPMiddleware or use app.add_middleware) that checks
Content-Length and/or streams the body up to the limit and returns 413 if
exceeded, and reference/replace the existing app.add_middleware(CORSMiddleware,
...) registration to ensure the size-guard runs prior to proxy_request().
In `@src/worthless/proxy/rules.py`:
- Around line 317-320: The cleanup code must not drop a per-key asyncio.Lock
while another coroutine may still hold it; update the cleanup logic (the
periodic block that calls self._cleanup(now) and inside _cleanup) to only remove
entries from self._locks when the existing lock is not currently held (use
lock.locked() to skip deletion if True) and avoid replacing/removing locks in
other cleanup paths (the spots around the calls to self._cleanup, and the other
cleanup locations handling self._locks referenced in _load_limit and related
code). Concretely, before deleting self._locks[key] or creating a new lock for
the same key, check the existing lock with if existing_lock.locked():
continue/skip removal so you never evict a lock that may be in-use by an
awaiting request.
---
Duplicate comments:
In `@README.md`:
- Line 7: The README's Tests badge has an empty link target
("[]()") which triggers MD042; update the badge line by replacing
the empty parentheses with a valid CI/status badge URL (or remove the link
wrapper so the image is not a link) to ensure the badge points to a real
resource — locate the Tests badge Markdown line in README.md and change the link
target accordingly.
In `@src/worthless/proxy/app.py`:
- Around line 173-178: The shutdown sequence currently calls await
client.aclose(), await db.close(), and repo.close() sequentially so that if any
of those raise the zeroization loop over settings.fernet_key never runs; wrap
the teardown in nested try/finally blocks (or a single try with a finally) so
that regardless of exceptions from client.aclose(), db.close(), or repo.close()
the zeroization of settings.fernet_key always executes; locate the shutdown
block containing client.aclose(), db.close(), repo.close(), and the for i in
range(len(settings.fernet_key)) loop and move the zeroization into the finally
that is guaranteed to run after attempting to close client/db/repo, optionally
logging or re-raising errors after zeroization.
In `@src/worthless/proxy/metering.py`:
- Around line 195-208: The result() method fails to process any remaining
buffered partial line stored in _partial_line before computing usage, causing
undercounting when the stream ends; modify result() to first flush/parse the
buffered tail by invoking the same parsing logic feed() uses (e.g., call the
internal line-parsing routine or replicate its final-slice logic to handle
_partial_line) so that _found_usage, _input_tokens, _output_tokens (and for
openai, _total_tokens) reflect the last unterminated data: ensure you
update/emit usage from the parsed tail before returning UsageInfo in result(),
referencing result(), feed(), _partial_line, _parse_line (or the internal
parsing code), _found_usage, _input_tokens, _output_tokens, and _total_tokens to
locate and fix the logic.
In `@tests/test_cli_lock.py`:
- Around line 564-567: The test currently asserts shard keys via
repo.list_keys(), but the compensation path deletes the enrollment row
specifically; replace or augment that check to assert enrollments are gone by
calling the repository enrollment API: obtain the repo with _repo(home_dir) and
call asyncio.run(repo.list_enrollments()) (or the repository method that returns
enrollment rows) and assert it equals [] instead of relying on list_keys(); keep
or adjust the shard key assertion if desired.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 864e6f6b-bfcb-4933-9032-fedfa7375e7a
📒 Files selected for processing (25)
README.mdpyproject.tomlsrc/worthless/cli/bootstrap.pysrc/worthless/cli/commands/lock.pysrc/worthless/cli/keystore.pysrc/worthless/proxy/app.pysrc/worthless/proxy/config.pysrc/worthless/proxy/metering.pysrc/worthless/proxy/middleware.pysrc/worthless/proxy/rules.pysrc/worthless/storage/repository.pytests/test_adversarial.pytests/test_cli_lock.pytests/test_config.pytests/test_contract.pytests/test_docker_e2e.pytests/test_e2e_live.pytests/test_error_metering_and_hardening.pytests/test_fernet_bytearray.pytests/test_keystore.pytests/test_proxy.pytests/test_proxy_hardening.pytests/test_proxy_keyring.pytests/test_rules.pytests/test_streaming_metering.py
💤 Files with no reviewable changes (1)
- src/worthless/proxy/middleware.py
Comment on lines
33
to
35
| ```console | ||
| $ worthless wrap python -c "import os; print(os.environ.get('OPENAI_BASE_URL', 'not set'))" | ||
| OPENAI_BASE_URL=http://127.0.0.1:51799 | ||
| $ worthless wrap python app.py | ||
| ``` |
There was a problem hiding this comment.
Make this either a shell snippet or a real console transcript.
Using a console fence with a $ prompt but no output keeps MD014 firing. Either switch the block to bash and drop the prompt, or include the output lines.
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 34-34: Dollar signs used before commands without showing output
(MD014, commands-show-output)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 33 - 35, The fenced code block showing "$ worthless
wrap python app.py" should be converted to a proper shell snippet or a real
interactive transcript: either change the fence language from "console" to
"bash" and remove the leading "$ " prompt so it becomes a plain shell snippet,
or keep the "console" fence and add the command output lines below the prompt to
make it a real console transcript; update the triple-backtick block containing
the command accordingly.
| f"{base}/v1/messages", | ||
| json={ | ||
| "model": "claude-3-5-haiku-20241022", | ||
| "model": "claude-3-haiku-20240307", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Is Anthropic model claude-3-haiku-20240307 still supported in the Messages API, and what is the currently recommended low-cost model ID for live integration tests?
💡 Result:
No, the Anthropic model claude-3-haiku-20240307 (Claude Haiku 3) is not retired yet but is deprecated and scheduled for retirement on April 20, 2026 (some sources say April 19). As of today (2026-04-13), it is still functional in the Messages API, though no longer recommended due to reduced reliability. Migrate before the retirement date, when API requests will return errors like model_not_found or invalid_model. The currently recommended low-cost model for live integration tests is claude-haiku-4-5-20251001 (Claude Haiku 4.5), Anthropic's fastest and most cost-efficient active model. It is the direct replacement for claude-3-haiku-20240307, offering improved performance at similar low cost and speed, suitable for high-volume, latency-sensitive tasks.
Citations:
- 1: https://platform.claude.com/docs/en/about-claude/models/overview
- 2: https://claudelab.net/en/articles/api-sdk/claude-haiku-3-deprecation-migration
- 3: https://dev.to/peytongreen_dev/claude-3-haiku-is-being-deprecated-april-19-heres-how-to-find-and-fix-every-reference-in-your-1c14
- 4: https://platform.claude.com/docs/en/release-notes/overview
- 5: https://platform.claude.com/docs/en/about-claude/model-deprecations
- 6: https://docs.anthropic.com/en/docs/resources/model-deprecations
- 7: https://docs.anthropic.com/en/docs/about-claude/models/all-models
- 8: https://anthropic.com/claude/haiku
Migrate from deprecated Anthropic model claude-3-haiku-20240307 before April 20, 2026 retirement.
This model is scheduled to be retired in 7 days and API requests will return model_not_found errors afterward. Replace with claude-haiku-4-5-20251001 (Claude Haiku 4.5), Anthropic's current recommended low-cost model for integration tests. Consider parameterizing the model ID via environment variable or shared test constant to avoid future brittleness when models are rotated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_e2e_live.py` at line 63, The test currently hardcodes the
deprecated model id "claude-3-haiku-20240307"; replace that literal with the new
recommended id "claude-haiku-4-5-20251001" and update tests to read the model id
from a single source (e.g., an environment variable like TEST_ANTHROPIC_MODEL or
a shared constant TEST_MODEL_ID) so future rotations only require one change;
locate the string "claude-3-haiku-20240307" in tests/test_e2e_live.py and
refactor the code that sets the "model" field to use the env var or constant
with a sensible default of "claude-haiku-4-5-20251001".
Apply the same _free_port() → Docker-assigned port fix to the container fixture (not just persistent_container). Two parallel CI workflow runs share the same Docker daemon and race for ports. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tags, pre-cleanup Root cause: two workflow runs (push + PR events) race on the same Docker daemon, competing for ports and image tags. Fixes: - Add concurrency group to docker-security.yml (cancel duplicates) - Session-unique IMAGE_TAG prevents parallel builds clobbering - Pre-cleanup in all container fixtures removes stale leftovers - Respect WORTHLESS_DOCKER_IMAGE env var (skip build in CI) - Remove dead _free_port() function Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1. app.py: fernet zeroing in finally block — exception-safe shutdown 2. metering.py: flush _partial_line on result() — no lost usage data 3. metering.py: guard non-dict usage/message shapes — no crashes 4. config.py: close fd in finally — no fd leak on read failure 5. keystore.py: migrate returns False when store fell back to file 6. repository.py: _compute_decoy_hash raises after close() 7. lock.py: detect already-enrolled alias, don't re-enroll/destroy 8. README.md: fix empty badge URL 9. test_cli_lock.py: assert no enrollment row (not just no shard) 10. lock.py: move sys import to top of file Each fix has a corresponding test (TDD). 1170 tests pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Apr 16, 2026
This was referenced Apr 24, 2026
4 tasks
This was referenced Jul 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Seven P1 bugs stood between us and V1 release. Each was a correctness or security gap in the hot path — key material on disk, race conditions in rate limiting, half-enrolled state on crash, unbounded memory in streaming. This PR closes all seven, adds adversarial attack tests that prove each fix works, and addresses follow-up findings from four independent review agents.
Summary
fernet.keycleaned after keyring write, per-install keyring namespacing via SHA-256(home_dir), auto-migration from file→keyring on existing installsbytearraythroughout proxy chain with explicit zeroing on shutdown (SR-01/SR-02)asyncio.Lockaround sliding window read-check-update — no undercounting (security) or overcounting (false-positive UX)StreamingUsageCollectorextracts usage incrementally without buffering; fails closed with pessimistic 10k token estimate when extraction returns NoneReview agent findings (all addressed)
_lifespancontextTest plan
uv run pytest tests/— 228+ tests pass (excludes Docker e2e)tests/test_adversarial.py— 7 attack tests, all pass (attack = fails)tests/test_streaming_metering.py— 4 collector tests (OpenAI, Anthropic, no-buffer, no-usage)tests/test_keystore.py— 45 tests covering keyring namespacing, migration, cleanuptests/test_fernet_bytearray.py— 8 type contract teststests/test_rules.py— 48 tests including 3 concurrent burst tests🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests