Repository navigation
Codebase quality pass: security, performance, maintainability fixes - #1
Merged
Merged
Conversation
Project hooks loaded from repo-local .patchsmith/hooks.json were executed with shell=True, allowing shell metacharacters in untrusted config to chain arbitrary commands. Parse the command with shlex and run it as an explicit argv list (shell=False), blocking unparseable/missing commands with a clear reason. Adds regression tests. Co-authored-by: Tanzim Hossain Romel <tanhromel@gmail.com>
Session transcripts recorded full project-instruction and agent-profile text on every session start, resume, checkpoint, and config update. That text can contain secrets or proprietary content. Persist only metadata (paths and character counts) and reload the full text from disk on resume via the new rehydrate_config_instructions helper. Updates affected tests and adds a resume rehydration regression test. Co-authored-by: Tanzim Hossain Romel <tanhromel@gmail.com>
A hung git process (network or filesystem) could block a run indefinitely. Bound git calls in agent_apply, workflow workspace diff, and ingest (clone/checkout/query) with named timeout constants, treating timeouts the same as other git failures where applicable. Co-authored-by: Tanzim Hossain Romel <tanhromel@gmail.com>
…level - runtime/feedback.py compiled assertion/exception regexes once instead of per call over every stdout/stderr line. - model_clients.repair_plan_json_schema returns a module-level constant rather than rebuilding the dict on every model completion. - chat/routing.py builds the 90-entry natural-language route table and normalization regexes once at import time instead of on every input line. Co-authored-by: Tanzim Hossain Romel <tanhromel@gmail.com>
- workflow._merge_path_lists tracks seen paths in a set. - runtime.attempts.ineffective_target_paths uses per-signature sets and a global seen-set instead of repeated list membership tests. - retrieval graph neighbor filtering precomputes the repo path set once (repo_file_path_set) instead of scanning all files per neighbor. Co-authored-by: Tanzim Hossain Romel <tanhromel@gmail.com>
run_chat_task previously issued a live OpenAI models HTTP request on every task. Cache successful availability results on the chat runtime keyed by the requested model id; transient failures (auth/network/missing model) are still re-checked on the next task. Adds a cache-hit regression test. Co-authored-by: Tanzim Hossain Romel <tanhromel@gmail.com>
Prefer the diff reported by the agent runtime and only fall back to a workspace git diff when the agent did not report one (e.g. runtimes that edit the workspace directly). Avoids a git subprocess per retry attempt in the common path while preserving the fallback behavior. Co-authored-by: Tanzim Hossain Romel <tanhromel@gmail.com>
- Add a bounded, mtime/size-keyed file-text cache (cached_read_text) shared by retrieval and the code graph so the same files are not re-read on every retriever call or attempt. - Memoize build_code_context_graph per repo path with a fingerprint over the Python files' (path, mtime, size); the graph is rebuilt only when sources change. Removes the duplicate _safe_read in code_graph.py. - Adds a regression test that the graph rebuilds when sources change. Co-authored-by: Tanzim Hossain Romel <tanhromel@gmail.com>
read_transcript_rows previously re-read and re-parsed the entire JSONL file on every call; chat commands parse the same transcript several times between appends (e.g. /cost then /metrics, apply guard reads). Add a bounded cache keyed by (path, mtime_ns, size) that invalidates automatically on append. Adds a cache/invalidation regression test. Co-authored-by: Tanzim Hossain Romel <tanhromel@gmail.com>
…and attempts Introduce RuntimeTraceSnapshot (runtime/trace_snapshot.py) that extracts every latest-event signal (patch plan, quality, target/no-op/symbol violations, safety-gate rejection, patch target, old-span hash, mounted context paths) in a single reversed pass. - patch_plan_feedback_summary now does one pass instead of seven. - feedback_attempt_record builds one snapshot instead of ~six scans. - Duplicated _latest_* trace parsers across feedback.py and attempts.py now delegate to the shared snapshot, removing divergent copies. Adds snapshot unit tests; behavior is unchanged. Co-authored-by: Tanzim Hossain Romel <tanhromel@gmail.com>
code_graph.py duplicated _path_terms and _is_test_path from retrieval_features.py (with _path_terms subtly diverging on stopword filtering). Parameterize retrieval_features._path_terms with drop_stopwords and import both helpers into code_graph, preserving the graph's no-stopword-filter behavior while removing the duplicate definitions. Co-authored-by: Tanzim Hossain Romel <tanhromel@gmail.com>
The retry workspace baseline (.retry_baseline_repo) was only cleaned up on the success path, so a failed run could leave it on disk. Move cleanup into a finally block guarded on a nullable restorer reference. Co-authored-by: Tanzim Hossain Romel <tanhromel@gmail.com>
Introduce named constants for the sandbox command timeout and pids limit (sandbox.py), the per-attempt sandbox timeout and feedback truncation budget (runtime/attempts.py), and the retry resource-budget thresholds (workflow.py). No behavior change. Co-authored-by: Tanzim Hossain Romel <tanhromel@gmail.com>
…egistry runtime_from_transcript previously mutated ten locals through a long if/elif chain. Move the per-event logic into small handlers over a _ResumeAccumulator dataclass and dispatch via a registry, leaving the replay loop a few lines. Behavior is unchanged (covered by existing resume tests). Co-authored-by: Tanzim Hossain Romel <tanhromel@gmail.com>
Add a test asserting every primary chat command in the registry is documented in /help output, so newly registered commands cannot silently go undocumented. Co-authored-by: Tanzim Hossain Romel <tanhromel@gmail.com>
Co-authored-by: Tanzim Hossain Romel <tanhromel@gmail.com>
Co-authored-by: Tanzim Hossain Romel <tanhromel@gmail.com>
Co-authored-by: Tanzim Hossain Romel <tanhromel@gmail.com>
Co-authored-by: Tanzim Hossain Romel <tanhromel@gmail.com>
thromel
marked this pull request as ready for review
June 30, 2026 02:54
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.
Summary
A tracked, end-to-end code-quality pass over the codebase, addressing review findings in priority order (security → performance → maintainability → practices). Every change is committed as an isolated, logical step and the full quality gate is green.
Quality gate:
ruff check,ruff format --check,mypy src(292 files), andpytest(757 passed, 1 skipped) all pass.Security (P0)
shell=False— removes a shell-injection vector from repo-provided hook config; commands are parsed withshlex.splitand missing executables / unparseable commands are handled explicitly (regression tests added).*_chars) is recorded; full text is rehydrated from disk on session resume, keeping secrets/proprietary content out of logs.TimeoutExpired.Performance (P1)
RuntimeTraceSnapshotshared by feedback/attempts, replacing repeated reverse scans.git diffsubprocess on every retry attempt by preferring the agent'sfinal_diff.(model, endpoint)for the session.Maintainability (P1)
emit_summary/SummaryProtocol, sharedopen_urlHTTP helper, and a singlebudget_limit_label.is_test_path/path_termsare now public inretrieval_features, and an identical_is_test_pathduplicate incontext_packingwas removed.runtime.attemptsgod module: extracted the independent sandbox-attempt execution intoruntime/sandbox_attempt.py(clean separation of concerns), withattempts.pyre-exporting for API stability. The retry-feedback logic is intentionally kept together as a single cohesive unit.Readability & Practices (P2)
if/elifdispatch chain into a handler registry./helpagainst command-registry drift with a test.try/finally.raise ... from), and broad boundary handlers already record structured errors into trace/result objects — no redundant logging framework added.Deferred (recommended as isolated follow-up PRs)
These are large, purely-organizational reorganizations with non-trivial regression risk; keeping them out of this PR preserves reviewability:
deepagents_*.pyinto apatchsmith/deepagents/subpackage with a facade.evaluation_*/public_issue_*module navigation.Testing
uv run ruff check src tests— cleanuv run ruff format --check src tests— cleanuv run mypy src— clean (292 files)uv run pytest— 757 passed, 1 skipped