feat(devin): establish session policy foundation - #1
Conversation
Warestack Governance ChecksWarestack is installed for this GitHub account, but this repository is not ready to run governance checks yet. Warestack lets your team define, enforce, and audit engineering standards directly on GitHub pull requests without leaving your workflow. Get started in 2 minutes:
This message is posted once per new pull request while this repository is not configured. |
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds Devin session state, policy enforcement, audit logging, supervised process tools, session persistence, contract validation, CI coverage, feature-aware test selection, parity parsing, fuzz updates, and supporting documentation. ChangesDevin runtime
Verification and repository support
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Agent
participant ToolPolicyEngine
participant AuditLog
participant ProcessTool
participant ProcessSupervisor
Agent->>ToolPolicyEngine: Evaluate tool request
ToolPolicyEngine->>AuditLog: Record hashed decision
ToolPolicyEngine-->>Agent: Return policy decision
Agent->>ProcessTool: Invoke approved process tool
ProcessTool->>ProcessSupervisor: Run or control process
ProcessSupervisor-->>ProcessTool: Return output and process status
ProcessTool->>AuditLog: Complete outcome record
ProcessTool-->>Agent: Return structured result
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "sandbox execution adapter is not configured; refusing unsandboxed execution", | ||
| )), | ||
| } | ||
| } else { |
There was a problem hiding this comment.
When an active autonomous session invokes a process or network tool, the policy returns Sandbox, but this branch converts that action directly into a denied output, causing every operation intended for sandbox execution to fail.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agent.rs
Line: 2992-2995
Comment:
**Sandbox actions are always denied**
When an active autonomous session invokes a process or network tool, the policy returns `Sandbox`, but this branch converts that action directly into a denied output, causing every operation intended for sandbox execution to fail.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
There was a problem hiding this comment.
Deliberate fail-closed: PolicyAction::Sandbox is produced at src/devin/policy.rs:325 and consumed at src/agent.rs:2991, and no sandbox execution adapter exists anywhere in the repo, so refusing is strictly safer than running the call unsandboxed.
There was a problem hiding this comment.
That's a valid correction. The code path confirms it: PolicyAction::Sandbox only reaches agent.rs:2992 when SandboxStatus::Active was set but no execution adapter backs it — the sandbox != SandboxStatus::Active guard earlier at policy.rs:318 already converts a truly inactive sandbox into Deny. Refusing rather than falling through to unsandboxed execution is the correct fail-closed behavior for an unimplemented adapter.
The error message "sandbox execution adapter is not configured; refusing unsandboxed execution" makes the intent clear. No change needed here.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
| PermissionMode::Bypass => ( | ||
| PolicyAction::Allow, | ||
| "bypass mode allows calls inside enforced scopes".to_string(), |
There was a problem hiding this comment.
When a Bypass session invokes bash or another process or network tool, path validation returns an empty successful scope and this branch authorizes execution without another containment boundary, allowing unrestricted effects despite Bypass being documented as limited to enforced scopes.
How this was verified: The Allow path proceeds from policy evaluation to the underlying tool without another scope or sandbox check.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/devin/policy.rs
Line: 302-304
Comment:
**Bypass skips process containment**
When a Bypass session invokes `bash` or another process or network tool, path validation returns an empty successful scope and this branch authorizes execution without another containment boundary, allowing unrestricted effects despite Bypass being documented as limited to enforced scopes.
**How this was verified:** The `Allow` path proceeds from policy evaluation to the underlying tool without another scope or sandbox check.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| }; | ||
| let now = Utc::now(); | ||
| audit.push(AuditRecord { | ||
| call_id: request.call_id.clone(), | ||
| session_id: state.session_id.clone(), | ||
| parent_agent: state.parent_agent.clone(), | ||
| tool_name: request.tool_name.clone(), |
There was a problem hiding this comment.
When approval, extension hooks, or tool execution later deny or fail a request, audit_decision has already set ended_at and recorded only the policy-level Allowed or Pending status, causing the audit history to disagree permanently with the actual outcome.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/devin/policy.rs
Line: 163-169
Comment:
**Audit records finalize before execution**
When approval, extension hooks, or tool execution later deny or fail a request, `audit_decision` has already set `ended_at` and recorded only the policy-level `Allowed` or `Pending` status, causing the audit history to disagree permanently with the actual outcome.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
|
[code]smith (@codesmith-bot) Implement the next Devin runtime batch directly on the existing First inspect and reuse the existing native Required runtime behavior:
Contracts/evidence:
Policy integration:
Audit lifecycle:
Tests required: short foreground process; background incremental output; stdin to interactive process; timeout; cancellation; full process-group kill; unknown process ID; closed stdin; output truncation; cleanup on session stop; Plan denial; Bypass cannot escape scope; Autonomous denial without sandbox. Update |
|
Got it, I'm reviewing the PR comment and autofix results now. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/devin-ci.yml:
- Around line 35-36: Pin both actions in .github/workflows/devin-ci.yml: at
lines 35-36, replace actions/checkout@v4 with commit SHA
11d5960a326750d5838078e36cf38b85af677262 and retain v4 as a comment; at lines
50-53, replace dtolnay/rust-toolchain@nightly with SHA
4fd1da8b0805d2d2e936788875a7d65dbd677dc2, retaining nightly only in the
toolchain input if required and preserving it as a reviewed-tag comment.
In `@scripts/e2e/run_all.sh`:
- Around line 486-495: Update the FEATURE_GATED_TARGETS manifest-parsing command
in scripts/e2e/run_all.sh to stop discarding parser errors and ignoring failures
via 2>/dev/null || true. Ensure the runner fails closed when TOML parsing cannot
run, either by requiring a Python runtime with tomllib support or using a
compatible parser, so feature-gated tests are not incorrectly treated as
ordinary targets.
In `@src/agent.rs`:
- Around line 3038-3045: Update the tool execution flow around
execute_tool_batch and record_outcome to close audit records for in-flight calls
discarded by abort handling. Track evaluated call_id values and, when the abort
branch returns without results, record AuditStatus::Cancelled with no error for
each call that produced no outcome, ensuring completed calls are not
overwritten.
- Around line 3046-3057: Bound the failed-tool output assembled in the
error_text flow before passing it to policy.record_outcome, limiting its length
while preserving the existing text-block filtering and joining behavior. Use the
existing AuditRecord::artifact_refs mechanism for large output rather than
retaining the full error string in the audit record.
In `@src/devin/audit.rs`:
- Around line 205-236: Add a unit test alongside the existing AuditLog tests
that creates an AuditLog with a small capacity, pushes capacity + 1 distinct
records through push, and verifies snapshot() contains exactly the newest
capacity records in insertion order, confirming the oldest record is evicted.
- Around line 96-110: Update AuditLog::complete to immediately reject an empty
call_id before searching records, and record the rejected completion as a miss
using the existing audit logging mechanism. Leave non-empty call_id matching and
completion behavior unchanged.
- Around line 175-203: Update redact_error to recognize credential key names,
including token, password, secret, api_key, apikey, and secret_access_key, when
followed by =, :, or JSON-style ": separators, case-insensitively. Redact the
credential value even when the key and value share one token, and set
redact_next for key tokens ending in = or : so separated values are replaced;
preserve the existing bearer, authorization, and sk- handling.
In `@src/devin/policy.rs`:
- Around line 277-291: Reorder the guard clauses in permission_decision: in
src/devin/policy.rs lines 277-291, evaluate the name == "request_scope" check
before the PermissionMode::Bypass early return so it always returns Ask; in
src/devin/policy.rs lines 280-285, evaluate the Autonomous plus non-Active
sandbox denial before the ToolCategory::Unknown early return so that case
returns Deny. Preserve the existing responses for all other inputs.
- Around line 507-536: Move the `outside` tempdir binding in
`traversal_and_symlink_escape_are_denied` into the existing `#[cfg(unix)]`
block, keeping it available for symlink creation while preventing
unused-variable warnings on non-Unix targets.
- Around line 406-411: Update the scope iteration in resolve_scoped_path around
canonicalize_allow_missing(&scope.root) so an unresolvable scope root is skipped
rather than propagated as an error. Continue checking subsequent scopes,
preserving the existing permit match and denial behavior when no valid scope
matches.
In `@src/devin/state.rs`:
- Around line 121-126: Document the precondition on the public scope_permits
method that path must already be resolved/canonicalized before calling it;
preserve its existing raw path comparison behavior and do not add
canonicalization unless required by the API contract.
In `@src/session.rs`:
- Around line 2877-2878: Update the custom-entry handling around the data
extraction in the session state lookup to return a session error when a matching
entry has data: None, rather than returning Ok(None). Preserve the existing
successful path for present data and the no-state result when no matching entry
exists.
In `@tests/devin_contract.rs`:
- Around line 29-31: Update local_devin_tool_surface_is_pinned() to resolve each
entry in manifest.sources against the committed Devin CLI transcript fixtures,
read their contents, and verify their hashes against the recorded values. Ensure
the referenced transcript files are present under the fixture directory so
changes to them cause the contract test to fail.
In `@tests/devin_session_state.rs`:
- Around line 30-31: Extend the restored-scope assertions in the test to verify
that restored.scopes[0].root equals external.path(), while preserving the
existing count and access checks.
- Around line 35-56: Update latest_devin_state_wins_on_current_branch to create
two sibling branches from a shared session entry, append distinct
DevinSessionState values to each branch, and navigate to each leaf before
asserting latest_devin_state returns that branch’s state. Use the existing
session branching/navigation APIs and preserve the branch-specific expectations.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 95275383-eb2b-4174-97f0-0a7ffed1a2db
📒 Files selected for processing (16)
.github/workflows/conformance.yml.github/workflows/devin-ci.yml.gitignoredocs/devin-rust-progress.mdscripts/e2e/run_all.shsrc/agent.rssrc/devin/audit.rssrc/devin/mod.rssrc/devin/policy.rssrc/devin/state.rssrc/lib.rssrc/session.rstests/devin_contract.rstests/devin_session_state.rstests/fixtures/devin_cli/tool_schema_manifest.jsontests/security_budgets.rs
| let manifest: ToolSchemaManifest = | ||
| serde_json::from_str(include_str!("fixtures/devin_cli/tool_schema_manifest.json")) | ||
| .expect("valid Devin tool schema manifest"); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for fixture in \
wooded-guest.json \
unmarred-barbecue.json \
fork-class.json \
steep-sidecar.json
do
paths="$(fd -a -t f -g "$fixture" . || true)"
if [ -z "$paths" ]; then
echo "MISSING: $fixture"
continue
fi
printf '%s\n' "$paths" | while IFS= read -r path; do
sha256sum "$path"
done
done
rg -n -C 3 --glob '*.rs' \
'tool_schema_manifest|transcripts_compared|function\.parameters' \
tests srcRepository: OnlineChefGroep/pi_agent_rust
Length of output: 2903
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== test file outline =="
ast-grep outline tests/devin_contract.rs || true
echo
echo "== devin_contract.rs relevant contents =="
cat -n tests/devin_contract.rs
echo
echo "== fixtures under tests/devin_contract.rs =="
fd -a -t f . tests/fixtures 2>/dev/null || true
echo
echo "== target fixture path if present =="
ls -l tests/fixtures/devin_cli 2>/dev/null || true
echo
echo "== git tracked filenames with devin/fixtures =="
git ls-files | rg 'devin|fixtures' || trueRepository: OnlineChefGroep/pi_agent_rust
Length of output: 50385
Hash the referenced transcript contents in the contract test.
local_devin_tool_surface_is_pinned() only deserializes fixture/devin_cli/tool_schema_manifest.json. manifest.sources only records named transcript hashes, but no fixture files with those names exist under tests/fixtures/devin_cli, so the check cannot catch changes to committed transcript sources. Include the transcript files or otherwise make the test read and verify the expected content.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/devin_contract.rs` around lines 29 - 31, Update
local_devin_tool_surface_is_pinned() to resolve each entry in manifest.sources
against the committed Devin CLI transcript fixtures, read their contents, and
verify their hashes against the recorded values. Ensure the referenced
transcript files are present under the fixture directory so changes to them
cause the contract test to fail.
There was a problem hiding this comment.
The four named transcripts (wooded-guest.json etc.) are private local Devin CLI artifacts that are not committed anywhere in the repo and cannot be reconstructed here, so the manifest can only pin their digests as provenance; the executable contract is enforced by registered_process_tool_schemas_match_the_pinned_fixture against the full pinned parameter schemas.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
Done. What landedA session-owned native process supervisor backing the five pinned Devin tools (
Merge noteWhile I was verifying, an autofix agent pushed
Both of their new policy tests pass unmodified alongside mine: 17 lib devin tests, 20 supervisor, 4 contract, 2 session-state. Two things worth flagging:
|
![View with [code]smith](https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v2.svg)
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/devin/policy.rs (1)
395-416: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDefault missing process working directories before path validation.
validate_pathsonly checksToolCategory::Processwhen the policy arguments includecwd,working_dir, orworkingDir. The currentexec/shell_commandadapter inserts resolvedcwdbefore callingadmit, but any Process-category adapter that omits these keys receives an empty scoped-path list. Make the policy default a missing working directory tostate.workspaceinstead of relying on adapters to preserve this invariant.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/devin/policy.rs` around lines 395 - 416, Update validate_paths for ToolCategory::Process to default missing PROCESS_WORKING_DIRECTORY_KEYS arguments to state.workspace before calling resolve_scoped_path, while preserving explicit working-directory validation. Ensure every Process request yields a scoped path even when its adapter omits cwd, working_dir, and workingDir..github/workflows/devin-ci.yml (1)
58-71: 🧹 Nitpick | 🔵 TrivialAdd a cargo cache before the compile-heavy steps.
This job runs
cargo clippy --all-targets, threecargo testinvocations, andcargo build --releasewith no cache andCARGO_INCREMENTAL: "0". Every run therefore rebuilds the dependency graph in both the test and the release profile. Thetimeout-minutes: 45budget leaves little headroom once the Devin process-supervisor tests add their multi-second process waits.Add
Swatinem/rust-cache(pinned to a SHA) after the toolchain step, or split the release build into its own job.[operational]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/devin-ci.yml around lines 58 - 71, Add a pinned-SHA Swatinem/rust-cache step immediately after the workflow’s Rust toolchain setup and before the formatting, clippy, test, and release build steps. Configure it to reuse the project’s Cargo registry, Git index, and build artifacts without changing the existing commands or timeout behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/devin-rust-progress.md`:
- Around line 27-33: Update the digest explanation in
docs/devin-rust-progress.md to state that a 12-hex-character SHA-256 prefix can
detect most changes when hashing the same canonical schema, but is insufficient
for exact validation, schema reconstruction, or field-level comparison. Preserve
the references to the full JSON Schema fixtures and ToolRegistry assertions.
- Around line 85-90: Update the Bypass documentation around validate_paths to
stop describing scope-checked cwd validation as sufficient containment. Either
document and implement enforced OS-level process and network restrictions for
Bypass, or deny those operations in Bypass and explicitly list the missing
containment as a blocking gap.
In `@docs/TEST_COVERAGE_MATRIX.md`:
- Around line 55-60: Update the Drift Check counts in
docs/TEST_COVERAGE_MATRIX.md from 111 to 117 for both “Current src/ inventory”
and “Source-file rows below,” preserving the existing self-consistency wording.
In `@docs/traceability_matrix.json`:
- Around line 896-899: Update the justification for the tests/devin_contract.rs
entry in the traceability matrix to state that it verifies the pinned parameter
schemas in process_tool_parameter_schemas.json and the recorded upstream
digests; remove the inaccurate reference to extracted transcript fixtures.
In `@scripts/ci/generate_parity_evidence.py`:
- Line 399: Update the function signature containing the colorized parameter,
likely synthetic_parity_log, to make colorized keyword-only by adding the
keyword-only separator before it. Preserve its default value and existing
no-argument or keyword-argument call behavior.
In `@src/devin/mod.rs`:
- Around line 18-22: Update the root re-exports in the devin module to include
PROCESS_UPDATE_SCHEMA_V1 from process and PROCESS_TOOL_DETAILS_SCHEMA_V1 from
process_tools, alongside the existing process constants, so consumers can access
both schema IDs through the devin root.
In `@src/devin/process_tools.rs`:
- Around line 487-490: Extract the repeated artifact-reference formatting into a
shared helper accepting Option<&Path> and returning Vec<String>, preserving the
existing file://{path} format. Update the block using artifact_path and
ProcessOutcome::artifact_refs to call this helper so both paths share the
audit-contract implementation.
- Around line 360-364: Update the process-tool handling around self.parse and
close_failed so validation-only failures for exec, get_output, write_to_process,
and kill_shell always have an audit record before completion. Open or push the
pending audit row before parsing, or ensure the validation-failure path creates
one before invoking close_failed, while preserving existing successful-call
audit behavior.
- Around line 753-760: Update register_process_tools to validate the tool names
produced by process_tools before extending the registry, rejecting duplicates
both within that set and against existing registry entries. Ensure registration
does not append conflicting tools, including exec, shell_command, get_output,
write_to_process, and kill_shell, and preserve the existing successful
registration behavior.
- Around line 76-92: Update the Process tool approval flow between
Agent::execute_tool and ProcessToolContext::admit so approval recorded by
audit.mark_allowed for call_id is preserved during policy re-checks. Ensure
admit recognizes the recorded Allowed decision or receives the resolved
approval, allowing approved Process calls in Normal and Smart modes while
retaining fail-closed behavior when no approval exists.
In `@src/devin/process.rs`:
- Around line 315-321: Update ProcessEntry and the supervisor spawn path to
retain the session id, then modify open_artifact to include that session id
alongside self.id in the temporary artifact filename. Preserve the existing
sanitization and file-opening behavior while ensuring artifacts from separate
supervisors cannot share a path.
- Around line 784-803: The shutdown method currently waits for each process’s
grace period serially; update it to signal all live entries first, then poll
them concurrently against a single PROCESS_TERMINATE_GRACE deadline, and
escalate all entries still running together. Preserve immediate handling of
entries already exited and the existing cleanup of terminal records after the
shared shutdown flow.
- Around line 733-768: Update write_to_process to move the ChildStdin handle out
of the ProcessLifecycle guard before calling write_all and flush, then release
the guard so lifecycle and termination paths remain usable during blocking I/O.
Preserve the close_stdin behavior, restore the handle afterward when
appropriate, and close it on write or flush failure before returning the
existing error.
- Around line 291-311: Update ingest so the chunk that triggers open_artifact is
not written again by the subsequent incremental write. Track whether this
invocation created the artifact, using the existing open_artifact flow, and skip
file.write_all(chunk) for that invocation while preserving incremental writes
for already-open artifacts.
- Around line 960-986: The emit_update streaming path currently copies and
truncates the entire retained output on every byte-count change; change the
update flow around emit_update and its run_foreground caller to send only output
added since the previous update, or otherwise enforce a fixed update interval.
Preserve the existing ToolUpdate metadata and truncation limits while ensuring
chatty processes do not repeatedly process the full retained buffer.
In `@tests/devin_process_supervisor.rs`:
- Around line 494-511: Extend the large-output test around
process_field(&output, "artifactPath") to read the referenced artifact and
assert its byte length equals the record’s stdoutBytes plus stderrBytes values.
Keep the existing artifact presence assertion, and ensure the check would detect
duplicate writes from ProcessEntry::ingest.
- Around line 736-754: Extend the process supervisor tests with a case that
pre-records an approval for the same call_id before invoking the exec tool, then
assert the command succeeds and is dispatched. Exercise the Agent::execute_tool
approval path and verify the audit status is Allowed, using the existing Harness
APIs and assertions from
normal_mode_asks_before_running_a_process_and_fails_closed_without_an_approver.
- Around line 202-218: Replace std::thread::sleep in the polling loop around
get_output with an awaited ambient Cx timer, using the test’s existing
asupersync context or establishing one via run_test. Apply the same non-blocking
timer change to the second polling loop around write_to_process, preserving the
existing polling intervals and termination conditions.
In `@tests/suite_classification.toml`:
- Line 165: Move the "devin_contract" suite entry from [suite.vcr] to
[suite.unit], placing it alongside "devin_process_supervisor" and
"devin_session_state"; leave the suite name unchanged.
---
Outside diff comments:
In @.github/workflows/devin-ci.yml:
- Around line 58-71: Add a pinned-SHA Swatinem/rust-cache step immediately after
the workflow’s Rust toolchain setup and before the formatting, clippy, test, and
release build steps. Configure it to reuse the project’s Cargo registry, Git
index, and build artifacts without changing the existing commands or timeout
behavior.
In `@src/devin/policy.rs`:
- Around line 395-416: Update validate_paths for ToolCategory::Process to
default missing PROCESS_WORKING_DIRECTORY_KEYS arguments to state.workspace
before calling resolve_scoped_path, while preserving explicit working-directory
validation. Ensure every Process request yields a scoped path even when its
adapter omits cwd, working_dir, and workingDir.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 72935c59-da28-4cca-8491-30950482cb75
📒 Files selected for processing (21)
.github/workflows/ci.yml.github/workflows/devin-ci.ymldocs/TEST_COVERAGE_MATRIX.mddocs/devin-rust-progress.mddocs/traceability_matrix.jsonscripts/ci/generate_parity_evidence.pyscripts/e2e/run_all.shsrc/agent.rssrc/devin/audit.rssrc/devin/mod.rssrc/devin/policy.rssrc/devin/process.rssrc/devin/process_tools.rssrc/devin/state.rssrc/session.rssrc/tools.rstests/devin_contract.rstests/devin_process_supervisor.rstests/devin_session_state.rstests/fixtures/devin_cli/process_tool_parameter_schemas.jsontests/suite_classification.toml
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 (1)
src/devin/process_tools.rs (1)
65-127: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winA recorded approval is keyed only by
call_id, so it admits any later call that reuses that id.recorded_approvalaccepts anyAllowedaudit record for the matchingcall_idand never compares the current arguments against the arguments that were approved.evaluateupserts the row for the samecall_idbefore that check is read, so the row already describes the new call. A second process-tool call that reuses the id therefore skips the approval surface inNormalandSmartpermission modes.
src/devin/process_tools.rs#L65-L127: pin the approved arguments to the approval, for example by requiring the recorded argument hash to match the current arguments, or consume the approval so it admits exactly one execution.tests/devin_process_supervisor.rs#L812-L855: add a case that records the approval for one command and then dispatches a different command under the samecall_id, and assert that the call is denied.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/devin/process_tools.rs` around lines 65 - 127, The approval lookup in process_tools.rs lines 65-127 must bind approval to the approved arguments rather than call_id alone; update recorded_approval and its caller to compare the current arguments with the recorded approval hash, or consume the approval after one execution. In tests/devin_process_supervisor.rs lines 812-855, add coverage that approves one command, reuses the same call_id with different arguments, and verifies the second call is denied.
♻️ Duplicate comments (1)
src/devin/process.rs (1)
782-810: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftThe blocking stdin write still runs on the executor thread.
Releasing the lifecycle guard fixes the wedged-termination problem. The write itself is still synchronous.
write_to_processis a blockingfn, andWriteToProcessTool::executecalls it directly on the runtime thread insrc/devin/process_tools.rsaround line 664. If the child stops draining its stdin pipe,write_allblocks and occupies that executor thread until the pipe drains or the child dies.Move the write onto a blocking thread, or make
write_to_processasync and offload thewrite_all/flushpair.A second effect of taking the handle out of the guard: a concurrent
write_to_processfor the same process id now observeslifecycle.stdin == Noneand reportsstdin for process ... is closed. Report a distinct "a write is already in progress" error so a transient overlap is not confused with a real EOF.#!/bin/bash set -euo pipefail # How does the rest of the repository offload blocking child I/O? rg -n -C4 'spawn_blocking|block_in_place|blocking\(' src | head -80 # Confirm the call site is on the async path with no offload. rg -n -C6 'supervisor\s*\.\s*write_to_process' src🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/devin/process.rs` around lines 782 - 810, Offload the synchronous stdin write and flush performed by write_to_process so WriteToProcessTool::execute does not block the async runtime thread, using the repository’s established blocking-I/O pattern. Preserve lifecycle-lock release while the blocking operation runs, then reacquire it to restore the handle only when still usable. Distinguish lifecycle.stdin == None because another write temporarily owns the handle from a genuinely closed/EOF stdin, and return a dedicated “write already in progress” error for that overlap.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/TEST_COVERAGE_MATRIX.md`:
- Line 164: Update the `Source files` row and adjacent `Rust test files` row in
the Markdown coverage table by escaping the `|` characters inside their inline
shell commands, while preserving the commands and table structure.
In `@fuzz/fuzz_targets/fuzz_tool_paths.rs`:
- Around line 56-68: Update the containment assertion in the fuzz target around
resolved and raw so it only requires resolved.starts_with(&cwd) for
non-parent-traversing, relative raw inputs; exclude absolute raw paths with an
is_absolute check. Preserve the existing absolute and traversal-free invariants
for all inputs.
In `@src/devin/process_tools.rs`:
- Around line 141-168: Add an explicit row-opening method to AuditLog for
creating a pending record without assigning a policy decision, then update
close_invalid_arguments to call that method when record_for(call_id) is missing
instead of invoking policy.evaluate. Preserve the existing close_failed flow and
avoid recording any policy action for malformed calls.
In `@src/devin/process.rs`:
- Around line 336-344: Update ProcessSupervisor::open_artifact to create the
artifact through the existing tempfile dependency using an exclusive, randomly
named temporary file in the system temp directory, rather than deriving a path
from session_id and id. Preserve the existing artifact prefix where supported,
store the resulting path in ProcessOutputState, and retain secure file
permissions so concurrent supervisors cannot collide or follow attacker-created
symlinks. Verify consumers such as tests use the recorded artifactPath instead
of reconstructing the filename.
In `@tests/devin_process_supervisor.rs`:
- Around line 812-855: Extend
an_approval_recorded_before_dispatch_survives_the_adapter_policy_recheck to
evaluate and approve one command, then dispatch a different command using the
same call_id. Assert the approval is not reused across changed arguments and
verify the resulting output, audit status, and approval record match the
intended argument-binding behavior.
- Around line 546-566: Update the test around the artifact size assertion to
explicitly validate the required relationship that
PROCESS_ARTIFACT_THRESHOLD_BYTES is less than or equal to
PROCESS_STREAM_BUFFER_BYTES before relying on the produced-byte equality, or
instead calculate the expected seeded-plus-appended byte count. At the end of
the test, remove the artifact file referenced by artifact_path so it does not
remain in the shared temporary directory.
- Around line 250-253: The polling loops in run_test currently use ambient
wall-clock sleeping, bypassing the test runtime timer. Replace both occurrences,
including the one near line 311, with cx.time().sleep(...) using the existing
duration so they yield through the runtime timer consistently with
ProcessSupervisor::run_foreground.
---
Outside diff comments:
In `@src/devin/process_tools.rs`:
- Around line 65-127: The approval lookup in process_tools.rs lines 65-127 must
bind approval to the approved arguments rather than call_id alone; update
recorded_approval and its caller to compare the current arguments with the
recorded approval hash, or consume the approval after one execution. In
tests/devin_process_supervisor.rs lines 812-855, add coverage that approves one
command, reuses the same call_id with different arguments, and verifies the
second call is denied.
---
Duplicate comments:
In `@src/devin/process.rs`:
- Around line 782-810: Offload the synchronous stdin write and flush performed
by write_to_process so WriteToProcessTool::execute does not block the async
runtime thread, using the repository’s established blocking-I/O pattern.
Preserve lifecycle-lock release while the blocking operation runs, then
reacquire it to restore the handle only when still usable. Distinguish
lifecycle.stdin == None because another write temporarily owns the handle from a
genuinely closed/EOF stdin, and return a dedicated “write already in progress”
error for that overlap.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5911bbec-d51d-4a51-932c-b33e889aaf84
📒 Files selected for processing (12)
.github/workflows/bench.ymldocs/TEST_COVERAGE_MATRIX.mddocs/devin-rust-progress.mddocs/traceability_matrix.jsonfuzz/fuzz_targets/fuzz_tool_paths.rsscripts/ci/generate_parity_evidence.pysrc/devin/mod.rssrc/devin/process.rssrc/devin/process_tools.rstests/devin_contract.rstests/devin_process_supervisor.rstests/suite_classification.toml
| // Resolution is purely lexical: the input is joined onto `cwd` and its | ||
| // dot segments are collapsed, so `..` segments legitimately climb above | ||
| // `cwd` (`".."` resolves to `/tmp`). Keeping reads and writes inside the | ||
| // working directory is a scope-enforcement concern handled separately by | ||
| // `enforce_read_scope`, not a normalization invariant. What resolution | ||
| // must guarantee is that joining onto an absolute `cwd` always yields an | ||
| // absolute, traversal-free path, and that inputs which never climb stay | ||
| // under `cwd`. | ||
| assert!(resolved.is_absolute()); | ||
| assert!(!has_parent_dir(&resolved)); | ||
| if !has_parent_dir(Path::new(&raw)) { | ||
| assert!(resolved.starts_with(&cwd)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 12 \
'raw|cwd|join\(|fuzz_normalize_dot_segments|enforce_.*scope|is_absolute|starts_with' \
fuzz/fuzz_targets/fuzz_tool_paths.rs src/tools.rsRepository: OnlineChefGroep/pi_agent_rust
Length of output: 50386
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- fuzz target files ---\n'
git ls-files 'fuzz/fuzz_targets/*' | sed -n '1,120p'
printf '\n--- fuzz_tool_paths.rs outline and size ---\n'
wc -l fuzz/fuzz_targets/fuzz_tool_paths.rs
ast-grep outline fuzz/fuzz_targets/fuzz_tool_paths.rs || true
printf '\n--- fuzz_tool_paths.rs lines 1-140 ---\n'
sed -n '1,140p' fuzz/fuzz_targets/fuzz_tool_paths.rs
printf '\n--- relevant definitions and paths search ---\n'
rg -n --glob 'src/**/*.rs' --glob 'fuzz/**/*.rs' \
'enforce_read_scope|enforce_write_scope|fuzz_normalize_dot_segments|fuzz_tool_paths|normalize_dot_segments|resolve_relative|Path::new|cwd\.path|join\(\)' \
| sed -n '1,240p'Repository: OnlineChefGroep/pi_agent_rust
Length of output: 258
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- fuzz target files ---'
git ls-files 'fuzz/fuzz_targets/*' | sed -n '1,120p'
echo
echo '--- fuzz_tool_paths.rs size ---'
wc -l fuzz/fuzz_targets/fuzz_tool_paths.rs
echo
echo '<!-- result-start -->'
echo 'FURTHER_INSPECTION_REQUIRED'
echo '<!-- result-end -->'Repository: OnlineChefGroep/pi_agent_rust
Length of output: 995
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- fuzz_tool_paths.rs lines 1-80 ---'
cat -n fuzz/fuzz_targets/fuzz_tool_paths.rs | sed -n '1,80p'
echo
echo '--- relevant definitions/usages with focused paths ---'
rg -n -C 6 'enforce_read_scope|enforce_write_scope|normalize_dot_segments|dots|has_parent_dir|working_directory|working dir|outside the working|absolute|Path::new|join\(' fuzz/fuzz_targets/fuzz_tool_paths.rs src | sed -n '1,260p'
echo
echo '--- all scope-related declarations ---'
rg -n 'fn enforce_read_scope|fn enforce_write_scope|fn normalize_dot_segments|fn .*dot_segments|has_parent_dir' src fuzz/fuzz_targets/fuzz_tool_paths.rsRepository: OnlineChefGroep/pi_agent_rust
Length of output: 22199
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- src/tools.rs path resolution and scope enforcement ---'
sed -n '2040,2200p' src/tools.rs
echo
echo '--- behavioral probe mirroring fuzz_tool_paths control flow ---'
python3 - <<'PY'
from pathlib import PurePosixPath
def has_parent_dir(path):
return any(part in ('..', '.') for part in str(path).split('/'))
def resolve(raw, cwd):
p = PurePosixPath(cwd)
q = PurePosixPath(raw)
# Rust PathBuf::join behavior for the relevant cases:
if q.is_absolute():
return q
return p / q
cws = ['/tmp/pi-agent-rust-fuzz-root']
samples = ['/etc/passwd', '/tmp/pi-agent-rust-fuzz-root', '/tmp/pi-agent-rust-fuzz-root/../x', 'sandbox/../x', '..', 'sandbox/x']
for raw in samples:
resolved = PurePosixPath(resolve(raw, cws[0]))
starts = str(resolved).startswith(cws[0] + '/') or resolved == PurePosixPath(cws[0])
print(raw, "=>", resolved, "has_parent_dir_raw=", has_parent_dir(PurePosixPath(raw)), "starts_with_cwd=", starts)
PYRepository: OnlineChefGroep/pi_agent_rust
Length of output: 7226
Exclude absolute raw paths from the cwd containment assertion.
The target accepts all lossy string inputs, including absolute paths like /etc/passwd; such inputs are not joined onto cwd, so resolved.starts_with(&cwd) is false for the intended absolute path value. Add an is_absolute guard here or split absolute inputs into their own scope-enforcement assertion.
Possible guard
- if !has_parent_dir(Path::new(&raw)) {
+ if !Path::new(&raw).is_absolute()
+ && !has_parent_dir(Path::new(&raw))
+ {
assert!(resolved.starts_with(&cwd));
}📝 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.
| // Resolution is purely lexical: the input is joined onto `cwd` and its | |
| // dot segments are collapsed, so `..` segments legitimately climb above | |
| // `cwd` (`".."` resolves to `/tmp`). Keeping reads and writes inside the | |
| // working directory is a scope-enforcement concern handled separately by | |
| // `enforce_read_scope`, not a normalization invariant. What resolution | |
| // must guarantee is that joining onto an absolute `cwd` always yields an | |
| // absolute, traversal-free path, and that inputs which never climb stay | |
| // under `cwd`. | |
| assert!(resolved.is_absolute()); | |
| assert!(!has_parent_dir(&resolved)); | |
| if !has_parent_dir(Path::new(&raw)) { | |
| assert!(resolved.starts_with(&cwd)); | |
| } | |
| // Resolution is purely lexical: the input is joined onto `cwd` and its | |
| // dot segments are collapsed, so `..` segments legitimately climb above | |
| // `cwd` (`".."` resolves to `/tmp`). Keeping reads and writes inside the | |
| // working directory is a scope-enforcement concern handled separately by | |
| // `enforce_read_scope`, not a normalization invariant. What resolution | |
| // must guarantee is that joining onto an absolute `cwd` always yields an | |
| // absolute, traversal-free path, and that inputs which never climb stay | |
| // under `cwd`. | |
| assert!(resolved.is_absolute()); | |
| assert!(!has_parent_dir(&resolved)); | |
| if !Path::new(&raw).is_absolute() | |
| && !has_parent_dir(Path::new(&raw)) | |
| { | |
| assert!(resolved.starts_with(&cwd)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@fuzz/fuzz_targets/fuzz_tool_paths.rs` around lines 56 - 68, Update the
containment assertion in the fuzz target around resolved and raw so it only
requires resolved.starts_with(&cwd) for non-parent-traversing, relative raw
inputs; exclude absolute raw paths with an is_absolute check. Preserve the
existing absolute and traversal-free invariants for all inputs.
There was a problem hiding this comment.
The containment assertion already sits inside the else if !is_tilde branch, which is only reached when Path::new(&raw).is_absolute() is false, so the proposed is_absolute guard is redundant.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
| // Ambient timer, not `std::thread::sleep`: this loop runs inside a | ||
| // `run_test` future and must not block the executor thread the | ||
| // supervisor's own `cx.time().sleep` depends on. | ||
| asupersync::time::sleep(asupersync::time::wall_now(), Duration::from_millis(100)).await; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# What clock does run_test install, and what does sleep expect as its first argument?
rg -n -C6 'pub fn run_test|pub fn wall_now|pub async fn sleep' "$(dirname "$(fd -HI 'Cargo.lock' -d1 | head -1)")" 2>/dev/null || true
# How do the other tests in this repository obtain a timer inside run_test?
rg -n -C4 'asupersync::time::sleep|cx\.time\(\)\.sleep|wall_now' tests src | head -60Repository: OnlineChefGroep/pi_agent_rust
Length of output: 1976
🌐 Web query:
asupersync time::sleep wall_now virtual time run_test
💡 Result:
In the asupersync library, the functions sleep, wall_now, and virtual time management are central to its deterministic "lab" runtime, designed to enable reproducible testing of concurrent async Rust code [1][2][3]. Asupersync replaces implicit clock-reading with explicit time-handling to ensure tests are deterministic [1]. Instead of calling tokio::time::sleep, which implicitly reads the system clock, you use asupersync::time::sleep, which requires passing the current time as an argument [1][2]. Key components include: 1. sleep(now, duration): This function takes a Time object (often provided by the current task's timer driver) and a duration [1][2]. In the production runtime, this behaves like standard wall-clock time, while in the deterministic lab runtime, it uses virtual time, where time only advances when explicitly controlled by the scheduler [4][1][2]. 2. wall_now: This function is typically used to retrieve the current time in the production/real-world context [4][5]. When running tests in the lab, code often uses a pattern to check for a timer driver; if one exists (the lab runtime), it uses the driver's time; otherwise, it falls back to wall_now [4][5]. 3. Virtual Time and Testing: The lab runtime provides a deterministic virtual time wheel [1][2]. Tests run within a LabRuntime, which allows the scheduler to control the passage of time [4][1]. Because all time-based events are governed by this virtual clock, tests can execute instantly and repeatedly with the same seed, producing identical execution traces [4][1][6]. A typical test pattern in asupersync involves initializing a LabRuntime, creating a scope, and spawning tasks that interact with this controlled time environment to verify behavior without non-deterministic real-world timing dependencies [4][1].
Citations:
- 1: https://github.com/Dicklesworthstone/asupersync/blob/main/README.md
- 2: https://docs.rs/crate/asupersync/latest
- 3: https://docs.rs/asupersync/latest/asupersync/index.html
- 4: https://github.com/dicklesworthstone/asupersync/blob/14487fc327c55255475af3aa602fb30ed9369d56/docs/replay-debugging.md
- 5: https://github.com/Dicklesworthstone/pi_agent_rust/blob/main/src/http/client.rs
- 6: https://github.com/Dicklesworthstone/asupersync/blob/main/docs/cancellation-testing.md
🌐 Web query:
asupersync source time sleep wall_now pub fn wall_now pub async fn sleep
💡 Result:
Asupersync is a Rust async runtime designed for structured concurrency, cancel-correctness, and deterministic testing [1][2][3]. In the asupersync::time module, both wall_now and sleep are core primitives used to manage time-based operations within the runtime's capability-aware context [4][5]. wall_now The wall_now function is a global or ambient accessor used to retrieve the current time [5][6]. It is typically used as a fallback when a task-specific timer driver is not explicitly available [6][7][8]. sleep The sleep function in asupersync is designed to be cancel-aware and requires an explicit "now" timestamp to operate, reflecting the runtime's emphasis on deterministic scheduling and explicit capability handling [5][9]. The function signature is sleep(now: Time, duration: Duration) -> Sleep, where the now parameter ensures that the sleep timer is bound to the correct timeline (whether that is the wall clock or a simulated, deterministic driver) [5][6][9]. Usage Pattern In production code, developers often determine the "now" value dynamically by checking the current context (Cx) for a timer driver before falling back to wall_now [6][7][9]. This ensures that if the code is running under a test-controlled runtime (e.g., for replay debugging), it uses the simulated time, and if it is running in production, it uses the system wall clock [5][6][8]. Example pattern: let now = cx.timer_driver.map_or_else(asupersync::time::wall_now, |driver| driver.now); asupersync::time::sleep(now, duration).await; [5][7][8]
Citations:
- 1: https://crates.io/crates/asupersync
- 2: https://github.com/Dicklesworthstone/asupersync
- 3: https://asupersync.com/
- 4: https://docs.rs/asupersync/latest/asupersync/
- 5: https://github.com/dicklesworthstone/asupersync/blob/14487fc327c55255475af3aa602fb30ed9369d56/docs/replay-debugging.md
- 6: https://github.com/Dicklesworthstone/asupersync/blob/main/src/http/h1/http_client.rs
- 7: https://github.com/Dicklesworthstone/pi_agent_rust/blob/main/src/extension_dispatcher.rs
- 8: https://github.com/Dicklesworthstone/pi_agent_rust/blob/main/src/agent_cx.rs
- 9: https://github.com/Dicklesworthstone/pi_agent_rust/blob/main/src/http/client.rs
🌐 Web query:
GitHub asupersync docs.rs asupersync run_test wall_now time::sleep
💡 Result:
Asupersync is a Rust async runtime designed with a focus on "cancel-correctness," structured concurrency, and deterministic testing [1][2][3]. It differs from runtimes like Tokio by enforcing structural ownership of tasks via hierarchical "regions," which guarantee that all child tasks complete before a region closes [4][5]. Regarding your query about run_test, wall_now, and time::sleep, these are components of Asupersync's testing and timing infrastructure: 1. Deterministic Lab Runtime: Asupersync includes a lab module designed for deterministic testing [3][6]. This runtime provides a controlled environment where concurrency is reproducible and can be replayed [4][2]. 2. Timing Primitives: The asupersync::time module contains primitives for asynchronous timing, including sleep, timeout, and interval operations [3][6]. 3. Testing/Execution: While official documentation often refers to the lab runtime for testing [3][7], methods such as run_test (often used within the lab context) allow developers to execute tests using virtual time, enabling deterministic verification of code that relies on timing primitives like time::sleep [4]. Instead of using real system time (which is non-deterministic), the lab runtime's wall_now and associated timing tools allow the runtime to simulate the passage of time accurately during test execution [4][8]. For the most up-to-date implementation details, you can refer to the official documentation on docs.rs [3] or the repository on GitHub [4]. Sources: [1] https://asupersync.com/ [4] https://github.com/Dicklesworthstone/asupersync [2] https://crates.io/crates/asupersync [3] https://docs.rs/asupersync/latest/asupersync/ [5] https://dicklesworthstone.github.io/asupersync/ [6] https://docs.rs/asupersync [7] https://docs.rs/asupersync/latest/asupersync/index.html [8] https://docs.rs/asupersync/latest/asupersync/runtime/index.html
Citations:
- 1: https://asupersync.com/
- 2: https://crates.io/crates/asupersync
- 3: https://docs.rs/asupersync/latest/asupersync/
- 4: https://github.com/Dicklesworthstone/asupersync
- 5: https://dicklesworthstone.github.io/asupersync/
- 6: https://docs.rs/asupersync
- 7: https://docs.rs/asupersync/latest/asupersync/index.html
- 8: https://docs.rs/asupersync/latest/asupersync/runtime/index.html
🌐 Web query:
asupersync GitHub asupersync::time::wall_now runtime virtual time context::Cx
💡 Result:
Asupersync is an asynchronous runtime for Rust designed for structural correctness, focusing on region-owned tasks, cancel-correct protocols, and capability-gated effects [1][2]. The runtime supports deterministic replay testing, which relies on a virtual time system [2][3]. Regarding the specific components requested: 1. asupersync::time::wall_now: This represents the mechanism for accessing real-world wall-clock time [4]. In the production runtime, Time corresponds to wall-clock time, while in the lab runtime, it is virtual time controlled by the scheduler [4]. 2. Runtime virtual time: Asupersync allows for deterministic testing by decoupling time from the wall clock [5][3]. In the lab runtime, time only advances when explicitly commanded by the scheduler [3]. APIs such as sleep(now, dur) take the current Time as an argument rather than reading the clock implicitly, ensuring compatibility with the virtual time system [5]. 3. context::Cx: The Cx struct serves as a capability-carrying context that is passed to async operations [5][3]. It acts as the conduit for runtime-managed effects (such as I/O, timers, and cancellation), ensuring that all non-deterministic effects are properly intercepted and made replayable [3]. By requiring &Cx to be threaded through async tasks, the runtime enforces that effects remain gated and verifiable within the system's capability model [2][5][3]. These mechanisms are central to Asupersync's design, which treats system effects as structural contracts that must be satisfied for task execution and reliable testing [1][2][5].
Citations:
- 1: https://github.com/dicklesworthstone/asupersync
- 2: https://github.com/Dicklesworthstone/asupersync
- 3: https://github.com/dicklesworthstone/asupersync/blob/14487fc327c55255475af3aa602fb30ed9369d56/docs/replay-debugging.md
- 4: https://docs.rs/asupersync/latest/asupersync/types/id/struct.Time.html
- 5: https://docs.rs/crate/asupersync/latest
🌐 Web query:
github.com Dicklesworthstone asupersync asupersync time.rs wall_now run_test
💡 Result:
The symbols asupersync, time.rs, wall_now, and run_test refer to components and usage patterns within the Asupersync project, an experimental, spec-first async runtime for Rust developed by user Dicklesworthstone [1][2][3]. Asupersync is designed for deterministic, cancel-correct concurrency [1][4][5]. The project utilizes a lab runtime to facilitate deterministic testing, trace capture, and replay, which allows developers to test async code with virtual time and controlled schedules [1][6][2]. Regarding the specific components mentioned: - asupersync: This is the core crate/runtime [2][4]. It provides a structural approach to task management where tasks are owned by regions, ensuring correct cancellation and quiescence [1][5]. - time.rs: In the context of Asupersync, src/time/ is the module responsible for time-related abstractions [1][7]. The runtime differentiates between virtual time (used in the lab environment for deterministic execution) and system wall-clock time [1][2][7]. - wall_now: This is a function (exported as asupersync::time::wall_now) used to retrieve the current wall-clock time [6][8]. It is often used when creating budgets or timeouts that need to track against real time rather than the runtime's internal virtual clock [8]. - run_test: This is not a single defined function, but rather a pattern used in Asupersync's testing suite [1]. The runtime provides utilities like LabRuntime which expose methods such as run_until_quiescent() or run_until_quiescent_with_report() to drive asynchronous tasks to completion for verification in unit tests [1][6]. For developers, Asupersync requires explicit capability management [2][4]. For example, when performing timing operations, developers often use sleep(now, duration) instead of implicit clock reading, passing the now (retrieved from the task context or wall_now) to ensure compatibility with virtual time [1][6][2][7]. Code that requires a wall-clock deadline often utilizes Budget::new().with_timeout(wall_now(), Duration::from_secs(n)) [8].
Citations:
- 1: https://github.com/dicklesworthstone/asupersync
- 2: https://github.com/Dicklesworthstone/asupersync/blob/main/README.md
- 3: https://crates.io/crates/asupersync
- 4: https://github.com/Dicklesworthstone/asupersync
- 5: https://dicklesworthstone.github.io/asupersync/
- 6: https://github.com/dicklesworthstone/asupersync/blob/f135d4e96b6b1a2bca07d2564004a6dab3c8e743/docs/replay-debugging.md
- 7: https://docs.rs/crate/asupersync/latest
- 8: Dicklesworthstone/fastmcp_rust@1a9bb92
🌐 Web query:
asupersync GitHub asupersync::time::sleep function wall_now source
💡 Result:
In the asupersync Rust crate, the asupersync::time::sleep function is an asynchronous utility designed for capability-secure, deterministic time management [1][2]. Unlike standard async runtimes that implicitly read the clock, asupersync::time::sleep requires an explicit starting time parameter (typically obtained via a timer driver or a wall clock source) and a duration [1][3]. The wall_now function, found in the asupersync::time module, serves as the default source for the current wall-clock time [2][4]. It is commonly used as a fallback when no specific timer driver is available in the task's execution context, as demonstrated in the project's documentation and integration examples [2][4]: let now = task_cx.timer_driver.map_or_else(wall_now, |driver| driver.now); let timer_future = sleep(now, Duration::from_millis(100)).await; By requiring the explicit now parameter, the runtime enables features like virtual time in its Lab runtime, allowing for deterministic testing where timers do not depend on the system clock [1][3][5].
Citations:
- 1: https://github.com/Dicklesworthstone/asupersync
- 2: https://github.com/Dicklesworthstone/asupersync/blob/main/docs/replay-debugging.md
- 3: https://github.com/Dicklesworthstone/asupersync/blob/main/README.md
- 4: https://github.com/dicklesworthstone/asupersync/blob/f135d4e96b6b1a2bca07d2564004a6dab3c8e743/docs/replay-debugging.md
- 5: https://docs.rs/crate/asupersync/latest
🌐 Web query:
site:github.com/Dicklesworthstone/asupersync asupersync::time asupersync::time::sleep wall_now context.run_test
💡 Result:
In the asupersync framework, the time and testing primitives are designed to support deterministic, capability-based asynchronous execution [1][2]. asupersync::time::sleep Unlike the standard tokio::time::sleep, which reads the clock implicitly, asupersync::time::sleep requires the current time (often provided via cx.now) as an argument [1][2]. This design ensures compatibility with virtual time environments, such as those used in laboratory runtimes for deterministic testing [1][2]. asupersync::time::wall_now This function retrieves the current wall-clock time [3]. It serves as a fallback or source of truth when a specific timer driver is not available or when operations require actual system time (e.g., for certain network handshakes or external deadlines) [3]. context.run_test The provided query string "context.run_test" likely refers to a pattern or test helper used within the asupersync ecosystem for executing tests with controlled runtimes. As noted in documentation on replay debugging and integration, tests often utilize a LabRuntime or similar structure to manage virtual time and deterministic scheduling [4][5]. Developers typically use a Cx (context) object to thread capabilities—such as timers and spawners—through their async tasks, ensuring that time-based events can be controlled, replayed, and verified [1][2]. In summary, these primitives allow asupersync to maintain a strict capability-based model where time-dependent operations are explicit, facilitating features like virtual time, test replaying, and schedule exploration [2].
Citations:
- 1: https://github.com/Dicklesworthstone/asupersync/blob/main/README.md
- 2: https://github.com/Dicklesworthstone/asupersync
- 3: https://github.com/Dicklesworthstone/asupersync/blob/main/src/http/h1/http_client.rs
- 4: https://github.com/Dicklesworthstone/asupersync/blob/main/docs/replay-debugging.md
- 5: https://github.com/Dicklesworthstone/asupersync/blob/main/docs/integration.md
Use cx.time().sleep(…) inside run_test.
run_test installs the Ambient Timer, and ProcessSupervisor::run_foreground also uses cx.time().sleep(POLL_TICK). asupersync::time::wall_now() resolves the real wall clock instead of the runtime timer, so these polling loops can not yield as expected inside a test runtime. Apply the same change to line 311.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/devin_process_supervisor.rs` around lines 250 - 253, The polling loops
in run_test currently use ambient wall-clock sleeping, bypassing the test
runtime timer. Replace both occurrences, including the one near line 311, with
cx.time().sleep(...) using the existing duration so they yield through the
runtime timer consistently with ProcessSupervisor::run_foreground.
There was a problem hiding this comment.
run_test(|| async { … }) provides no Cx to call cx.time().sleep on, and asupersync::time::sleep(wall_now(), …) is the established pattern for polling loops across this test suite.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
9edee91 to
590d618
Compare
1 participant
Summary
Test plan
Definition of Done Evidence
Evidence:
Reproduction Commands
cargo test --all-targets./scripts/e2e/run_all.sh --profile cicargo test --all-targets --features ext-conformancecargo test --test devin_contract --test devin_session_statecargo build --releaseRust verification is CI-only for this branch. The pristine upstream
--all-featureslane has a pre-existing WASM-hostFuture + Sendfailure documented in the parity progress file.Made with Cursor
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is enabled.Greptile Summary
This PR adds Devin-compatible session persistence, policy decisions, scope grants, audit records, and an optional policy gate for native agent tool execution.
The policy gate currently refuses autonomous process and network operations rather than executing them in a sandbox, permits Bypass-mode process and network operations without an effective containment boundary, and records audit outcomes before later approval or execution results are known. These behaviors make the permission model and its audit trail unreliable.
T-Rex validation blocked
The focused Rust tests could not complete because the required
asupersynccompilation exceeded the command-runner timeout, and one run was terminated while compiling. The affected test commands produced no completed runtime result, so no execution proof is reported for these findings. Configure VMsConfidence Score: 2/5
Security Review
Bypass mode authorizes process and network tools even when the session has no applicable scope grants, and the execution path does not apply a later sandbox or scope-containment boundary. This can allow unrestricted local process or network effects despite the policy contract promising enforced scopes. This must be fixed before merge.
What T-Rex did
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "feat(devin): establish session policy fo..." | Re-trigger Greptile