fix: address autoresearch review follow-ups - #45
Conversation
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_354a2e77-e781-48e4-85f3-7cc30adcff62) |
![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: e837faaf6d
ℹ️ 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".
| ensure_status_unchanged(&self.workspace, &candidate_status, "validation or metric") | ||
| .await?; |
There was a problem hiding this comment.
When the agent has already modified or created a path, a validation or metric command can rewrite that same path while git status --porcelain remains identical (for example, both snapshots are still M src/foo.rs or ?? new.rs). Git's short-format documentation defines these entries as status codes plus paths, not content snapshots, so this check passes and an accepted experiment can commit changes made by the measurement command itself, invalidating the result. Compare content/index state, such as hashes or diffs, instead.
Useful? React with 👍 / 👎.
| match evaluation { | ||
| Err(error) if is_budget_error(&error) => return Err(error), |
There was a problem hiding this comment.
When the finite wall-clock budget expires during validation or measurement, this direct return bypasses both restore_checkpoint and restore_ignored_state; the later ensure_status_unchanged(...)? has the same problem when a command changes the status. The run therefore exits with the agent's candidate and command side effects still in the workspace, and a subsequent --resume immediately fails the clean-workspace precondition. Route these error exits through the same cleanup path used for rejected iterations and cover every such exit with a failing cleanup case.
AGENTS.md reference: AGENTS.md:L526-L528
Useful? React with 👍 / 👎.
| ensure_experiment_state(workspace, branch, commit).await?; | ||
| restore_checkpoint(workspace, commit).await?; |
There was a problem hiding this comment.
If a baseline validation or metric command checks out another branch or moves HEAD, ensure_tracked_state_unchanged correctly rejects it, but the cleanup path calls this same state check again before performing any reset. That check necessarily fails for exactly these violations, so restore_checkpoint is never reached and the workspace remains on the command-selected branch or commit despite the documented baseline restoration guarantee.
Useful? React with 👍 / 👎.
| "apollo-autoresearch-ignored-{}", | ||
| uuid::Uuid::new_v4() | ||
| )); | ||
| tokio::fs::create_dir_all(&backup_dir).await?; |
There was a problem hiding this comment.
On a multi-user Unix host, create_dir_all creates this directory under the shared system temp directory using the process umask, commonly making it searchable by other users. The snapshot explicitly contains ignored local credentials, and copy preserves modes such as a typical 0644 .env; a private workspace can therefore have its secrets exposed through /tmp/apollo-autoresearch-ignored-*, with the directory also surviving a crash because cleanup relies on Drop. Create the backup root atomically with owner-only permissions and constrain backup-file permissions as well.
AGENTS.md reference: AGENTS.md:L110-L114
Useful? React with 👍 / 👎.
| impl Drop for IgnoredWorkspaceState { | ||
| fn drop(&mut self) { | ||
| let _ = std::fs::remove_dir_all(&self.backup_dir); | ||
| } |
There was a problem hiding this comment.
When each snapshot is dropped, this synchronous recursive filesystem operation runs directly on the async runtime thread. A snapshot may contain 64 MiB and has no file-count bound, so a workspace with many small ignored files can stall the runtime for a substantial period at every iteration boundary. Perform cleanup through Tokio before dropping the state, or delegate the unavoidable synchronous cleanup to spawn_blocking.
AGENTS.md reference: AGENTS.md:L97-L99
Useful? React with 👍 / 👎.
| ":(exclude)vendor", | ||
| ":(exclude)vendor/**", |
There was a problem hiding this comment.
When an ignored vendor/ tree supplies dependencies to the build, excluding it from both capture and restoration lets an agent or validation command modify dependency source permanently across rejected iterations. If the same iteration also changes a tracked file, the metric can be accepted and recorded against a vendor mutation that is absent from the acceptance commit, so the ledger's supposedly reproducible best commit does not reproduce its metric. Either snapshot these inputs or make cleanup rebuild/remove them before each measurement.
Useful? React with 👍 / 👎.
| ) -> anyhow::Result<String> { | ||
| let mut command = tokio::process::Command::new("git"); | ||
| command.args(args).current_dir(workspace); | ||
| crate::tools::child_proc::scrub(&mut command); |
There was a problem hiding this comment.
In CI or wrappers that configure Git through GIT_CONFIG_COUNT, GIT_CONFIG_KEY_0, and GIT_CONFIG_VALUE_0, the generic scrubber removes GIT_CONFIG_KEY_0 because its name contains KEY but preserves the count and value. Git requires every configured entry to have both fields, as described in its environment configuration documentation, and exits with missing config key GIT_CONFIG_KEY_0; consequently autoresearch fails on its first Git command in these environments. Preserve the complete tuple or remove the count, keys, and values together.
Useful? React with 👍 / 👎.
1 participant
Follow-up to the merged autoresearch PR #44, addressing the second review pass.
Implemented:
Validation:
The full workspace test/release sweep was not run because AGENTS.md warns that building apollo-ui can exhaust the machine disk; the targeted checks are green.
Open workspace in Conductor
Note
Medium Risk
Changes sit on git rollback, subprocess execution, and automation prompts for autoresearch—important safety boundaries—but behavior is mostly tightening guards and telemetry rather than new ambient capabilities.
Overview
Autoresearch gets a second safety pass: baseline and per-iteration validation/metric runs must leave branch,
HEAD, and trackedgit statusunchanged (failures restore baseline ignored state); shell/git children inherit scrubbed env, share the run wall-clock budget with process-group kills on timeout, and acceptance commits use--no-verifyplus an empty hooks path. Ignored-file rollback moves from in-memory bytes to temp-dir copies with per-file and total snapshot caps while skipping volatile roots liketarget/node_modules; rollbacks also reset submodules, ledger writes atomically replace on Windows, and in-workspace ledger paths normalize before containment checks.Restricted automation builds a slimmer system prompt (no
USER.md/MEMORY.md) viabuild_restricted_system_prompt.Cost reporting marks calls without configured model prices as unpriced (
pricing_known/pricing_complete, counts and model lists) instead of treating them as $0; HTTP/v1/state, Telegram cost output, and the usage tool expose when totals are incomplete. Docs describe the new autoresearch guarantees and limits.Reviewed by Cursor Bugbot for commit e837faa. Bugbot is set up for automated code reviews on this repo. Configure here.