Repository navigation
Conversation
|
Thanks for opening your first pull request in the Codna CLI! Please make sure the checks are green and that the change follows CONTRIBUTING.md. A maintainer will review it shortly. |
![codna-ai[bot]](https://avatars.githubusercontent.com/in/4061960?s=60&v=4){kind=small}
| out = git_ok( | ||
| runtime, | ||
| tree, | ||
| ["log", "--all", "--fixed-strings", "--format=%H", f"--grep={message}", "-n", "1"], | ||
| ) | ||
| if not out or not out.strip(): | ||
| return None | ||
| return out.strip().splitlines()[0].strip() or None |
There was a problem hiding this comment.
🔴 HIGH · correctness — git log --grep substring match causes false idempotency hits
In _existing_commit, the commit message for turn N (e.g. "codna: turn run:task:1") is searched with git log --all --fixed-strings --grep=<message> -n 1. Since --grep performs a substring match (even with --fixed-strings), the message for turn 1 ("...task:1") is a prefix substring of the message for turn 10 ("...task:10"). If turn 10 has already been committed and turn 1 is retried by Temporal, _existing_commit will return turn 10's sha instead of None, causing checkpoint_turn to skip creating turn 1's real commit and instead assign turn 10's sha to turn 1's ref. This corrupts the audit trail: turn 1's ref points to turn 10's tree, and future operations build on the wrong commit.
| out = git_ok( | |
| runtime, | |
| tree, | |
| ["log", "--all", "--fixed-strings", "--format=%H", f"--grep={message}", "-n", "1"], | |
| ) | |
| if not out or not out.strip(): | |
| return None | |
| return out.strip().splitlines()[0].strip() or None | |
| To fix, verify the returned sha's commit message matches exactly: after finding a candidate sha, check `git log -1 --format=%s <sha>` equals the expected message. Alternatively, append a unique terminator to the message (e.g., a NUL byte or a suffix like `\n[codna-checkpoint]`) but that changes history format. |
Reply @codna fix and Codna opens a verified, test-green, risk-gated fix PR.
| def _reload_tail_under_lock(self) -> None: | ||
| """Refresh the cached tail from the FILE, under the lock, before computing `seq`. | ||
|
|
||
| Called on every append. On the first call it is exactly `_ensure_loaded`; on later calls it is the | ||
| difference between a linear chain and a fork, because another instance may have appended since this one | ||
| loaded. The dedupe set is rebuilt too: an event this instance never saw may already be in the file, and | ||
| honouring the idempotency key is the other half of what the lock is for. | ||
|
|
||
| Deliberately re-reads rather than trusting in-memory state — the file is the source of truth and it is | ||
| shared. The cost is one sequential read of a local append-only file, which is what `_iter_records` already | ||
| does for the first load. | ||
| """ | ||
| last_hash = GENESIS_HASH | ||
| last_seq = 0 | ||
| seen: set[tuple[str, int]] = set() | ||
| for record in _iter_records(self._path): | ||
| last_hash = record.hash | ||
| last_seq = record.seq | ||
| if record.session_id is not None and record.event_index is not None: | ||
| seen.add((record.session_id, record.event_index)) | ||
| self._last_hash = last_hash | ||
| self._last_seq = last_seq | ||
| self._seen = seen | ||
| self._loaded = True |
There was a problem hiding this comment.
🟡 MEDIUM · performance — Full ledger file scan on every append causes O(n²) append cost
_reload_tail_under_lock reads the entire ledger file from the beginning on every single call to append(). For a long run with N ledger records, each append does O(N) work, making the overall append cost O(N²). For a run with thousands of turn events (a full LangGraph task can produce hundreds of events), this becomes significant: a 10,000-record ledger does 10,000 × full-scan on the last append. The comment acknowledges the full re-read but frames it as "one small tail read", which is incorrect — it re-reads from record 1 every time.
Reply @codna fix and Codna opens a verified, test-green, risk-gated fix PR.
| path = parts.path.rstrip("/") | ||
| netloc = parts.netloc | ||
| return f"{parts.scheme}://{netloc}{path}" |
There was a problem hiding this comment.
🟡 MEDIUM · security — URL reconstruction uses parts.netloc which preserves empty userinfo (@host)
In _validated_base_url, the credential check on line 131 tests parts.username or parts.password. For a URL like https://@host/path, parts.username is '' (empty string, falsy) and parts.password is None (falsy), so the check passes. However, parts.netloc is '@host', and line 137 reconstructs the URL as https://@host/path. The resulting URL with @ prefix in the netloc is an improperly formed URL that could be misinterpreted by some HTTP clients (treating the empty string before @ as userinfo). The URL should be reconstructed from parts.hostname and parts.port to strip any residual userinfo.
| path = parts.path.rstrip("/") | |
| netloc = parts.netloc | |
| return f"{parts.scheme}://{netloc}{path}" | |
| Replace lines 135-137 with: | |
| path = parts.path.rstrip('/') | |
| host = parts.hostname | |
| port = f':{parts.port}' if parts.port else '' | |
| return f"{parts.scheme}://{host}{port}{path}" |
Reply @codna fix and Codna opens a verified, test-green, risk-gated fix PR.
Automated mirror of cli/ (flattened), the root allowlist (server.json, glama.json, LICENSE, .gitleaks.toml) and the public-root/ community tree from thyn-ai/codna at 2b631dec6d3487d8d9c873d7215670ecc764e9ba. Merges itself via auto-merge once the mirror's required checks (codna review, Analyze (python)) pass and the codna-ai review approves; if a later sync lands first, this PR is closed as superseded -- every sync PR is a full snapshot of the exported surface.