Feat/codra agent native protocol#52
Conversation
…ovements Phase 1 - Core Agent Loop: - Tool-call sampling loop (LLM -> tool calls -> execute -> observe -> repeat) - Fixed file writes: actual content patching via search_replace tool - Token counting + auto-compaction at 85% context usage - Streaming support added to IntelligenceProvider trait Phase 2 - Tool System: - Parallel tool execution via FuturesUnordered-ready dispatcher - Per-file locking to prevent concurrent writes - Fixed repair loop with attempt tracking and budget management - Hook system with 7 event types (pre/post tool use, session, compact) Phase 3 - Session & Extensions: - Session actor pattern with channel-based event loop - Background task support + subagent spawning (GeneralPurpose/Explore/Plan) - LLM-driven plan generation (replaces heuristic-only) - Git workflow tools (branch, commit, status) Phase 4 - Polish: - Cost/token tracking across turns - History repair (dedup, orphan detection, synthetic results) - Command sandboxing (blocked patterns, allowlist, review-required) Tera API Integration: - TeraProvider in Rust (blocking + streaming, health check, model list) - TeraProvider in TypeScript (chat + streaming, device auth token) - ProviderKind::Tera added to protocol - Registered 'tera' in TypeScript provider factory Based on analysis of xAI Grok Build open-source repo (xai-org/grok-build).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7fc0fa72b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| // Block absolute paths and path traversal | ||
| if change.path.starts_with('/') || change.path.contains("..") { | ||
| if change.change_type.starts_with('/') || change.change_type.contains("..") { |
There was a problem hiding this comment.
When an approved task contains a path such as ../outside.txt with a normal change_type like modify, this guard no longer rejects it because it checks change_type instead of path. The later workspace_path.join(&change.path).starts_with(workspace_path) check is lexical and still returns true for workspace/../outside.txt, so TaskExecutor can create, modify, or delete files outside the workspace despite the approval boundary.
Useful? React with 👍 / 👎.
| let joined = self.root.join(rel); | ||
| if !joined.starts_with(&self.root) { |
There was a problem hiding this comment.
For fs.search_replace, a relative path like ../outside.txt passes this starts_with check because self.root.join(rel) retains the .. component, so the registered agent write tool can read and overwrite sibling files outside the workspace when that file exists. This should normalize/canonicalize the parent path or reject parent components before executing the write; the same lexical pattern should be audited in the read tool as well.
Useful? React with 👍 / 👎.
| try { | ||
| await readJsonResponse(startResponse, 'Tera auth start endpoint'); |
There was a problem hiding this comment.
If the device start endpoint returns a non-2xx JSON response, such as a 400 or 429 error, this call succeeds and consumes the response body, then execution falls through to parse the same Response again below. That produces a generic body-already-used failure instead of the intended HTTP error, so the non-OK branch should throw after reading the JSON error rather than continuing.
Useful? React with 👍 / 👎.
| "delete" => { | ||
| // Block delete by default for safety in MVP | ||
| return Err("Delete operations are blocked by default in MVP".to_string()); | ||
| if full_path.exists() { | ||
| fs::remove_file(&full_path).map_err(|e| e.to_string())?; | ||
| change.applied = true; |
There was a problem hiding this comment.
When an approved task has a changed_files entry with change_type: "delete", execution now removes the file, even though the protocol doc added in this commit still lists codra.file.delete as unsupported and the previous implementation blocked deletes. A model-generated delete can therefore erase workspace files after the normal task approval flow without a dedicated destructive-action gate.
Useful? React with 👍 / 👎.
| let stdout = String::from_utf8_lossy(&output.stdout).to_string(); | ||
| Ok(stdout) |
There was a problem hiding this comment.
For a pre_tool_use shell hook configured with fail_open: false, a policy command that exits non-zero (for example exit 1) still returns Ok(output) from Command::output, so dispatch falls through to Allow unless stdout happens to contain DENY. This means blocking hooks that fail closed do not actually block tool execution; check output.status and return an error on non-zero exits.
Useful? React with 👍 / 👎.
| const auth = JSON.parse(fs.readFileSync(AUTH_FILE, 'utf-8')); | ||
| return auth.accessToken || null; |
There was a problem hiding this comment.
When ~/.codra/auth.json contains an expired token, the auth module's getAuthToken() rejects it, but the new Tera provider reads accessToken directly and sends it anyway. Users configured with provider: tera will hit confusing 401/API failures instead of being prompted to log in again, so this path should share the existing expiry validation or check expiresAt before returning the token.
Useful? React with 👍 / 👎.
| case 'ollama': | ||
| return new OllamaProvider(config?.baseUrl); | ||
| case 'tera': | ||
| return new TeraProvider(config?.baseUrl); |
There was a problem hiding this comment.
When a user configures provider: "tera" with an apiKey in Codra config, this factory drops that key while OpenAI receives it. TeraProvider only checks CODRA_API_KEY and ~/.codra/auth.json, so configured Tera keys are ignored and requests fail with an authentication error unless the same key is duplicated in the environment.
Useful? React with 👍 / 👎.
| }; | ||
|
|
||
| let dispatcher: Arc<Mutex<dyn ToolDispatcher + Send + Sync>> = | ||
| Arc::new(Mutex::new(CodraToolDispatcher::new())); |
There was a problem hiding this comment.
Any spawned Explore or GeneralPurpose subagent currently gets an empty dispatcher, and AgentLoop only exposes tools registered in that dispatcher. As a result, subagent tasks that need to inspect or modify the requested workspace_path have no fs.read/search-replace tools available and can only hallucinate an answer instead of operating on the workspace.
Useful? React with 👍 / 👎.
| let arguments: serde_json::Value = | ||
| serde_json::from_str(&format!("{{{}}}", args_str)) | ||
| .unwrap_or(serde_json::Value::Object( | ||
| serde_json::Map::new(), | ||
| )); |
There was a problem hiding this comment.
When the model emits the common JSON-object form for tool calls, e.g. [TOOL_CALL: fs.read({"path":"Cargo.toml"})], this code wraps the object in another pair of braces and then silently falls back to {} on parse failure. The tool call is still executed but without required arguments, so valid-looking tool calls fail with missing-argument errors instead of running.
Useful? React with 👍 / 👎.
1 participant
No description provided.