Repository navigation
Audit findings: long functions as named steps, oversized files split by concern - #78
Merged
Merged
Conversation
…dmin AdminConfig::tools_in_effect lived in admin.rs and called back into ext_tools::defaults, while ext_tools::install_all and ::command called it: the two modules called each other for one piece of tool logic. It is now ext_tools::in_effect(&AdminConfig), beside the defaults it merges, and admin.rs no longer calls into ext_tools. Behaviour is unchanged; its tests now call the function where it lives.
install did everything in one 108-line function (resolving the release, deciding the expected sha256, swapping the version in, writing the manifest, removing old versions), and check folded the source and version rules into its own body. Each step is now a function named for what it decides, with the same messages; check's doc comment, which had drifted onto name_ok, is back on check. The end-to-end install tests are unchanged and pass.
… steps Sandbox::command was one 197-line function, and enter, the helper that applies the confinement, one 115-line one; sandbox_create and the proxy's read_head each folded several decisions into one body. - command: hidden_paths, windows_lease, start_proxy, environment, and RunIdentity for who the command runs as. - enter: enter_inner (the Windows second helper), helper_args, bind_to_parent, confine (limits, Landlock, seccomp). - sandbox_create: requested_network, refuse_hosts_above_ceiling, refuse_unenforceable. - read_head: head_lines, connect_head, forward_head. Order, cfg gates, strict and best-effort behaviour and every message are unchanged. Checked with the sandbox crate's tests (67 unit, 2 bin, 22 Windows escape tests) and clippy -D warnings for Windows, Linux (musl) and macOS (aarch64).
lib.rs held three things in 1362 lines: the parent side that builds and starts a confined run, the helper process that confines itself and execs the command, and the per-platform report of what is enforced. The helper is now helper.rs and the report status.rs; lib.rs (954 lines) keeps the Sandbox type and re-exports enter_if_requested, enforcement, Enforcement and Status, so every path callers use is unchanged. The code moved as is. Both modules get tests of their own: the helper's argument reading (spec, `--`, program; each malformed call refused with its reason) and a process not started as the helper carrying on; the report's gaps, its four lines in order, and the environment summary. Each was checked to fail with its line broken. clippy -D warnings for Windows, Linux (musl) and macOS (aarch64); sandbox tests 73 unit, 2 bin, 22 Windows escape.
AdminConfig::report built every section of `incode --admin` in one 149-line function. The roles and tier ladder, the budget and verify command, the output style and the plugins are now a function each; report lists them in order. The output is byte for byte the same: the admin tests pass, and `incode --admin` before and after the change was compared line by line (22 lines, identical).
admin.rs was 2131 lines: the settings and how they resolve, the types of the entries admin.json lists (API providers, agent CLIs, API models, the search service, the PDF printer, plugins), and the settings' tests. The entry types are now admin/entries.rs, re-exported from admin so every crate::admin:: path is unchanged, and the tests admin/tests.rs. admin.rs is 967 lines. Code and tests moved as they were: the same 32 admin tests pass, clippy -D warnings is clean for the workspace.
parse read the arguments and applied every policy flag in one 134-line match. Reading stays in parse; which options take a value is takes_value, and each flag is applied by apply_fs_flag, apply_net_flag, apply_env_flag or apply_limit_flag, with apply_flag naming one that none of them knows. Same flags, values and messages. A new test checks a flag of every area lands in its part of the policy and that bad values and unknown flags are refused; it fails with --pass-env taken out.
run_task_hooked set up a task run in one 173-line function. The operator hooks' messages (announce), the user_prompt_submit check (prompt_hook_context), holding the run to a custom command's tools (hold_to_command_tools), the turn's context block (turn_context) and the system context with the output style (system_context) are now functions; run_task_hooked keeps the order and the session_end that every way out runs. Same behaviour: the task_run tests (16) pass and workspace clippy -D warnings is clean.
settings_round_trip_through_the_file was 145 lines, most of them the nested values of a provider, two CLI agents, a model, a tool, a hook on every desktop event and one permission rule of each kind. Each is now a named helper beside the test. The AdminConfig literal itself still names every field with no ..Default, so a new setting fails to compile there until it is given a value that has to survive the file. Checked under sbx (writes confined to the repo, network denied): fmt, clippy -D warnings, admin tests 32/32. Control: saving a copy with desktop_hooks.stop cleared fails the test on "what was written must be what comes back".
custom_commands.rs was 1076 lines, 350 of them its test module. The tests move unchanged to src/custom_commands/tests.rs (347 lines, the same as before but for one level of indentation, which a byte-for-byte comparison confirmed; 18 tests before and after), leaving 728 lines of code, as admin.rs's tests moved in df6a746. Checked: fmt, clippy -D warnings, custom_commands tests 17/17 on Windows (the 18th is unix-only).
Under --net deny the command runs as the offline account, so every file it creates is owned by that account. The user's access to such a file is Modify (through Authenticated Users on a data drive), which holds no WRITE_DAC, so the next run's grant could not be propagated onto it: that run's command got "Access is denied" writing a file the previous run created (cargo's build cache on the second `cargo test`), and the earlier run's SID could not be revoked from it either. Each root now also carries one inheritable ACE for the user's own SID, READ_CONTROL | WRITE_DAC, what owning the file would give; it is added once (a root that already passes it down is left alone, sparing another walk of a large tree) and never revoked. The command gains nothing: it runs as the offline account, whose token does not hold the user's SID. Tests: escape_windows a_second_network_deny_run_writes_what_the_first_ created runs two network-deny runs on a folder with a data drive's ACL (under the profile an inherited full-control ACE hid the bug) and checks the first run's file is the offline account's; it fails with "Access is denied" without the fix. acl::tests checks the right is passed down, reaches a file made later, and is added once. sbx: 74 unit, 3 bin, 23 Windows escape tests; clippy -D warnings clean.
drive was 288 lines with a cyclomatic complexity of 32, building the candidate chain, each candidate's backend, its setup, the model choice, the turn, the session record, the memory engine's continuations, the sentinel, the unattended <ask> note and the fallback in one body. Each is now its own piece: Candidate (chain, agent_id, backend), prepare, choose_models, attempt, record_session, carry_on, with_unattended_note, and first_to_finish, which tries each candidate until one finishes. Failure::could_not_run and Failure::reported hold the wording of the two ways a candidate fails. Every message and the order of what is printed, traced and recorded is as before. The stray doc line "The instruction, through each usable agent..." is back on drive, not EndsEdits. New tests pin the fallback with fake candidates: the first to finish decides and the rest are never tried; a sentinel refusal ends the run; one failure is reported in its own words and several are listed by route; none at all says so. Control: stopping at the first failure fails two of them. task_run tests 19/19, clippy -D warnings clean.
task_run.rs was 1756 lines. Two cohesive blocks move out unchanged: - src/task_run/drive.rs: trying each usable agent until one runs the instruction (Task, EndsEdits, Candidate, prepare, choose_models, Failure, drive, first_to_finish, attempt, record_session, carry_on, with_unattended_note, show), with the fallback tests beside it. - src/task_run/tests.rs: the rest of the test module. task_run.rs keeps reporting, running a task with its hooks, and the prompt: 765 lines. Only Task and drive are visible to the parent. The move was checked line by line against the previous commit (496 lines each, identical but for pub(super) and one level of indentation); the only other change is task_run.rs importing AgentEvent alone. fmt, clippy -D warnings, task_run tests 19/19 (3 of them now in drive::tests).
…named steps cli/mod.rs held prepare_session and the REPL loop inline, each well over a thousand lines. prepare_session now lives in cli/prepare.rs and runs as named steps (open_workspace, load_admin, probe_tools, place_guards, place_intake, ...); the loop and its slash commands are cli/repl.rs and cli/repl/commands.rs. What both front ends share (the run backend, a typed line) stays in cli/mod.rs, now 707 lines. AgentSession gains set_extra_dirs so /add-dir can widen a session it holds in place. The Ctrl-C forwarder moved with the loop and now has the test it never had: forward_signals takes its signal source, so each_ctrl_c_stops_the_turn_until_the_listener_fails drives it with signals of its own (registered as the machine's watch check for it). Every function the old file had still exists; 16 tests are new. Tested: cargo check --all-targets, clippy --workspace --all-targets -D warnings, fmt, cargo test --lib 2368 passed before the forwarder test, cli:: 255 passed after it. Control: with cancel.cancel() removed the forwarder test fails at repl/tests.rs:276.
Merged with the branch's side kept where the two conflict: this branch moved the code main edited. main's changes to that code are carried into where it now lives: - task_run/drive.rs: the run's tool calls reach the engine's recorder (memory.tools_ran) after the turn and after each continuation (#77). - cli/repl.rs: the per-turn rules fetch into a list nothing read is gone; plan_turn fetches them (#76). - cli/tests.rs: the codex tests for the bridged default (#77). - admin/tests.rs: codex_tools resolves to incode unless set (#77). Tested: clippy --workspace --all-targets -D warnings, fmt, cargo test --lib 2414 passed.
Root cause: the code auditor flagged drive (235 lines) and run_prompt (216 lines), each over its 100-line function threshold, because each mixed several jobs in one body. drive built the session, made it ready, answered session/new or session/load, asked about unapproved servers, ran the prompt loop and ended the session. run_prompt read the prompt, dispatched slash commands, planned the turn, ran it and kept the books afterwards. drive (now 44 lines) calls build_session, hold, answer_opened, ask_about_servers, serve_prompts and end_session, in the order the one body ran them. run_prompt (now 76 lines) moves to acp_agent/prompt.rs as read_prompt, follow_the_client, run_command, plan, run_turn and after_turn. No behaviour changes: a panicked turn or a panicked apply still ends the thread without the session-end hooks, and lines announced late are still said between prompts. Tested: clippy --workspace --all-targets -D warnings, fmt, cargo test --lib 2414 passed. The existing acp flow tests cover every step.
…ested Root cause: the code auditor flagged acquire at 112 lines and cyclomatic complexity 21, over its thresholds of 100 and 20, because one body did root validation, the offline account's reads and parents filtering, the sweep, the rid claim, the record write and three grant loops. acquire (now 44 lines, about 10 branches) calls refuse_ungrantable, OfflineGrant::grantable, then Lease::grant_roots (grant_root per root), Lease::grant_reads and Lease::grant_parents, in the order the one body ran them. Each still lists a path on the lease before granting it, so a grant that fails part-way is taken back when the lease drops. That path had no test: a_grant_that_fails_part_way_takes_back_what_it_granted grants one root, fails on a second it may read but not re-ACL, and checks that the first carries no trace of the run's SID afterwards and that the record left for the sweep names only the second. It needs no admin rights. With the "list first" order reversed it fails. Tested: clippy --workspace --all-targets -D warnings, fmt, both acl tests, cargo test --lib 2414 passed. The incord-sandbox lib run has one failure that also happens without this change and only under the whole crate's parallel load: proxy::tests::plain_http_is_forwarded_and_private_answers_need_permission (connection reset at proxy.rs:838); proxy:: alone passes 3 of 3.
…r own modules Root cause: the code auditor flagged src/desktop/mcp.rs at 2,375 lines (threshold 1,000), with run_tool at 315 lines and call_tool at 131 (threshold 100), because one file held the JSON-RPC transport, the tool definitions, the per-call gate, every tool and the tests. mcp.rs (now 323 lines) keeps Server and its public items; the rest is mcp/transport.rs (the JSON-RPC loop), mcp/definitions.rs (what the model is told), mcp/gate.rs (the per-call checks), mcp/guard.rs (a tool's refusals before it acts), mcp/tools.rs (each tool as a step) and mcp/tests.rs (the same tests under the same names). call_tool (27 lines) runs catch_stop_file, authorise, hook_veto, outcome, tell_post_hooks and record_call in the order the one body did: a permission refusal wins over a stop, which wins over a hook veto, and a stop is still read twice. run_tool (22 lines) is the dispatch; the button parsing written out three times is one mouse_button. Public paths are unchanged (run_stdio and tool_definitions re-exported). Tested: clippy --workspace --all-targets -D warnings, fmt, desktop:: 169 passed before and after, cargo test --lib 2414 passed.
Root cause: the code auditor flagged AgentSession::turn_observed at 526 lines, over its 100-line function threshold, because one body opened the turn, ran it with failover to the standby agents, told the memory engine, handed back the follow-up, ran the engine's carry-on loop and the operator's stop hooks, held the questions, checked the sentinel and kept the accounts. A 12-argument closure carried the stream state. turn_observed (now 64 lines) moves to cli/agent_turn/turn.rs and calls open_turn, run_or_fail_over, tell_memory, follow_up, carry_on, operator_stop, hold_questions, check_sentinel and account in the order the one body ran them. TurnStream holds what the closure captured. The memory engine still hears tools_ran then said after the first run, the follow-up's said/tools_ran/said, and set_background/stop per continuation; failover still judges each agent once and swaps in the one that finished. agent_turn.rs goes from 2,784 to about 2,220 lines. New test, beside the code it drives (cli/agent_turn/turn/tests.rs): a_tool_result_is_drawn_on_rows_of_its_own_named_by_its_call, for the terminal tool-result panel no test reached. With the panel's start_new_line removed it fails at turn/tests.rs:76. Tested: clippy --workspace --all-targets -D warnings, fmt, cargo test --lib 2415 passed (cli:: 256 -> 257, acp_agent:: 65 -> 65).
On the windows-latest CI runner both ACL tests fail where they pass on a developer's machine: the data-drive folder already passes the user ACL rights (acl.rs, the_user_keeps_acl_rights...), and the locked root accepts its grant (a_grant_that_fails_part_way...). The runner's token is elevated, which changes who owns a new folder and what its owner may do; the tests' failure messages did not say which. They now name the user, whether the process is elevated, and the folder's owner and ACL, so the next run shows the cause rather than leaving it to a guess.
Root cause, from the diagnostics in f5e4207: the windows-latest runner is elevated (its user is runneradmin, S-1-5-21-...-500), and a folder it creates carries EXPLICIT full-control ACEs for that user and for BUILTIN\Administrators. The tests' setup took off the inherited ACEs (icacls /inheritance:r) and replaced only the SIDs it named (/grant:r), so those two stayed: the "data-drive" folder already passed the user everything, and the "locked" root could still be re-ACLed by an Administrator. Neither is a defect in the code under test. The setup now also takes off the user's own grants (both folders) and Administrators' (the locked root), with icacls /remove:g, before the lock, after which nobody here may change that ACL. On a developer's machine nothing changes: there were no such ACEs to remove. Tested here: both ACL tests pass 3 of 3; clippy -p incord-sandbox --all-targets -D warnings and fmt clean. The elevated case is proved by CI's windows-latest job.
Root cause: on windows-latest, a_second_network_deny_run_writes_what_the_first_created failed at escape_windows.rs:826 with an EMPTY owner. It read the owner with `powershell (Get-Acl ...).Owner`, and Windows PowerShell started from the runner's PowerShell 7 step inherits 7's PSModulePath, cannot load Get-Acl's module and prints nothing on stdout (the ACL tests' diagnostic showed the same empty owner). The test's and the diagnostic's powershell now start without PSModulePath, and print stderr too if it still says nothing. Also: the test's workspace drops the user's own explicit ACE, as the ACL unit tests now do. On an elevated runner a new folder carries one, and it would give the user WRITE_DAC beneath the root and hide exactly the case the test exists for. Tested here: the escape test runs (not skipped; offline account set up) and passes; both ACL unit tests pass; clippy -p incord-sandbox and fmt clean.
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
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.
The code auditor's findings in incode, fixed. Each long function becomes named steps, and each oversized file is split by concern. Behaviour does not change anywhere.
What changed
Long functions made of named steps (each was over the auditor's 100-line limit):
AgentSession::turn_observed: 526 → 64 lines, now incli/agent_turn/turn.rs. It is the turn loop: snapshot, run with failover, memory engine events, follow-up, carry-on loop, questions, sentinel, accounts.acp_agent:drive235 → 44 lines, andrun_prompt216 → 76, moved toacp_agent/prompt.rs.windows/acl.rs:acquire112 → 44 lines, with cyclomatic complexity 21 → about 10.task_run(driveand a run's setup), thesbxrun, helper and flag handling,ext_tools, and theadminsettings report.Files split:
cli/mod.rs(about 2,380 → 707 lines):prepare_sessionmoves tocli/prepare.rs, and the REPL tocli/repl.rsandcli/repl/commands.rs.desktop/mcp.rs(2,375 → 323 lines): the rest moves tomcp/{transport,definitions,gate,guard,tools,tests}.rs.admin,custom_commandsandtask_run.Kept exactly: the order of every side effect, including:
Public paths are unchanged.
One real bug fixed (
0a20564): on Windows, a secondsbx --net denyrun could not write files that the first run created.New tests:
Each new test fails with the behaviour it covers removed.
main merged in (#76, #77). Their edits to code this branch moved are carried to where that code now lives:
task_run/drive.rs,cli/repl.rs,cli/tests.rsandadmin/tests.rs.File-size limit
The owner set incode's file-size limit at 3,000 lines instead of the auditor's built-in 1,000. Files between 1,000 and 3,000 lines are left as they are.
brief.rs,agent/worker.rsandacp_agent/mod.rsare slightly over and accepted. Functions keep the 100-line limit.Tested
cargo fmt --all -- --checkcargo clippy --workspace --all-targets -D warningscargo test --lib: 2415 passed, 0 failedAll three are clean on this branch locally (Windows). The desktop suite (169) and the acp suite (65) are unchanged before and after.
Known flaky test, not from this branch:
incord-sandbox'sproxy::tests::plain_http_is_forwarded_and_private_answers_need_permissionsometimes resets its connection, but only when the whole crate runs in parallel on Windows. It passes when run alone. It is recorded as an open issue.