Repository navigation
Conversation
Restores, on top of the #152 revert (bb1b968), the behaviour changes that were bundled into the reorg. The resulting tree is identical to school/1nzr-reorg at a2afda4. - onboard.py + tests/test_onboard.py - a2a_bridge.py + tests/test_a2a_bridge.py - issue_bridge.py: --onboard CLI passthrough - issue_bridge.py: ENTIRE CRITICAL/HIGH veto on the sync path (+ test) - conductor.py: bookbag persistence of entire_findings/entire_status - src/entire_review.py: entire_replacement rename, --profile general, --format text dropped - review_packet.py: _sensor list/int findings normalization - docs/setup.md, docs/pipeline-explainer.md, campus.md, project_verify.yaml comment describing the above
Reviewer's GuideThis stacked behavior PR adds evidence-based repository onboarding and an independent OpenHands A2A bridge, exposes onboarding through the issue bridge, and strengthens Entire integration by enforcing CRITICAL vetoes—with optional HIGH strictness—across sync and async review flows. It also updates generated verification/context documentation and normalizes sensor result handling. Sequence diagram for onboarding a target repositorysequenceDiagram
actor Operator
participant IssueBridge
participant Onboard
participant TargetRepo
participant GitHub
participant Filesystem
Operator->>IssueBridge: --onboard --repo owner/repo
IssueBridge->>Onboard: main(onboard_argv)
Onboard->>TargetRepo: clone_repo(repo)
Onboard->>TargetRepo: _collect_evidence(root, info)
opt online metadata
Onboard->>GitHub: _github_repo_info(slug)
GitHub-->>Onboard: repository metadata
end
Onboard->>Onboard: generate_claude_md()
Onboard->>Onboard: generate_context_md()
Onboard->>Onboard: generate_project_verify()
opt --write
Onboard->>Filesystem: write_artifacts()
Filesystem-->>Onboard: generated L0-L4 artifacts
end
Onboard-->>IssueBridge: report and exit status
IssueBridge-->>Operator: onboarding report
Sequence diagram for A2A task dispatch and answer retrievalsequenceDiagram
actor Operator
participant A2ABridge
participant OpenHandsWebhook
participant AutomationAPI
participant AgentServer
Operator->>A2ABridge: --task task_text
A2ABridge->>A2ABridge: require_secrets()
A2ABridge->>OpenHandsWebhook: dispatch(webhook_url, secret, task)
OpenHandsWebhook-->>A2ABridge: run_id
loop until terminal status
A2ABridge->>AutomationAPI: poll_run_status(..., run_id)
AutomationAPI-->>A2ABridge: run status
end
A2ABridge->>AgentServer: read_agent_answer(..., conversation_id)
AgentServer-->>A2ABridge: assistant answer
A2ABridge-->>Operator: print answer
Sequence diagram for Entire veto in sync reviewsequenceDiagram
participant IssueBridge
participant Entire
participant AdversarialReview
participant ReviewPacket
IssueBridge->>Entire: run_entire_review(worktree_path, base_branch)
Entire->>Entire: subprocess.run(entire_path, review, --profile general, --base)
Entire-->>IssueBridge: findings
alt CRITICAL finding or ENTIRE_HIGH_STRICT=1 with HIGH finding
IssueBridge->>AdversarialReview: append entire_finding
IssueBridge->>AdversarialReview: set verdict FAIL
IssueBridge->>AdversarialReview: set score 0.0
end
IssueBridge->>ReviewPacket: create(...)
ReviewPacket->>ReviewPacket: _sensor(value)
ReviewPacket-->>IssueBridge: normalized finding count
Sequence diagram for async Entire findings and acceptancesequenceDiagram
participant Conductor
participant Entire
participant Bookbag
participant AcceptanceGate
Conductor->>Entire: run_entire_review(...)
Entire-->>Conductor: entire_findings, entire_status
Conductor->>Bookbag: locked_update_bookbag(..., entire_findings, entire_status)
Bookbag-->>Conductor: persisted sensor result
Conductor->>AcceptanceGate: _persist_acceptance(...)
AcceptanceGate->>Bookbag: read_bookbag(...)
Bookbag-->>AcceptanceGate: entire findings and status
alt CRITICAL finding
AcceptanceGate-->>Conductor: accepted = false
else ENTIRE_HIGH_STRICT=1 and HIGH finding
AcceptanceGate-->>Conductor: accepted = false
else no veto finding
AcceptanceGate-->>Conductor: evaluate judge verdicts and scores
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 26 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="issue_bridge.py" line_range="2694-2713" />
<code_context>
+ _entire_critical = [
</code_context>
<issue_to_address>
**Critical findings do not reject runs**
When a sync review has an authoritative canonical packet and Entire reports a CRITICAL finding or a strict HIGH finding, the sync path changes only `adversarial_review`, leaving the accepted `canonical_packet` unchanged. Later grading and publication use that packet, so the run continues despite the finding.
Update or invalidate the authoritative packet when the sync finding requires rejection, so downstream grading and publication reject the run.
</issue_to_address>
### Comment 2
<location path="conductor.py" line_range="658-664" />
<code_context>
+ # CRITICAL entire finding could not veto acceptance. Write them here,
+ # repo-namespaced to match read_bookbag/_persist_acceptance.
+ try:
+ locked_update_bookbag(
+ leaf.bead,
+ repo=target_repo or "__global__",
+ lock_timeout=10.0,
+ entire_findings=entire_findings,
+ entire_status=entire_status,
+ )
+ except Exception as e: # noqa: BLE001
+ print(f" ⚠ Entire review result persist failed: {e}")
</code_context>
<issue_to_address>
**Acceptance misses sensor findings**
When the bookbag lock cannot be acquired within the timeout while the Entire result is being persisted, `locked_update_bookbag` returns `None` without raising, and the caller proceeds to `_persist_acceptance` with the Entire findings unwritten. The acceptance gate can therefore accept work with a CRITICAL finding.
Check the update result and prevent acceptance from proceeding unless the Entire findings were persisted or otherwise included in the gate.
</issue_to_address>
### Comment 3
<location path="a2a_bridge.py" line_range="215" />
<code_context>
+ parser.error("--task is required for full pipeline")
+
+ print(f"Dispatching task: {args.task}", file=sys.stderr)
+ resp = dispatch(WEBHOOK_URL, WEBHOOK_SECRET, args.task)
+ print(f"Dispatch response: {resp}", file=sys.stderr)
+
</code_context>
<issue_to_address>
**Missing credentials reach requests**
When either `A2A_WEBHOOK_SECRET` or `A2A_SESSION_KEY` is unset, `main` does not call `require_secrets` before dispatch or polling, so the module-level empty values reach the requests. The bridge reports an authentication failure instead of the documented configuration error, and an endpoint accepting empty credentials receives unauthenticated requests.
Call `require_secrets` before dispatching or polling.
Also at `a2a_bridge.py:33-34`, `a2a_bridge.py:216`, `a2a_bridge.py:226`, `a2a_bridge.py:248-249`.
</issue_to_address>
### Comment 4
<location path="onboard.py" line_range="516" />
<code_context>
+ readme = ""
+ for name in ("README.md", "README.rst", "README.txt", "readme.md"):
+ if name in files:
+ readme = _read_text(root / name)
+ break
+ if not readme:
</code_context>
<issue_to_address>
**Repository symlinks escape checkout**
When an onboarded repository contains a symlink at a read or generated-output path and the operator runs onboarding, a malicious checkout can make `_read_text` follow a tracked README symlink to a host file and copy its contents into generated artifacts; symlinks at output paths such as `CLAUDE.md` also make `write_text` overwrite files outside the checkout. Reject symlinks and verify resolved paths stay under the intended root before reading or writing.
Reject symlinks and ensure each resolved input and output path remains inside its intended checkout or output root.
Also at `onboard.py:1486`.
</issue_to_address>
### Comment 5
<location path="onboard.py" line_range="1585" />
<code_context>
+ elif clone:
+ from src.core.repo_reader import clone_repo
+ repo_path = clone_repo(repo)
+ slug = repo
+ else:
+ raise OnboardError(
</code_context>
<issue_to_address>
**Onboarding triggers arbitrary requests**
When the selected repository has a JavaScript manifest and a lockfile referencing an attacker-controlled or internal URL, `onboard_repo` calls `clone_repo`, which automatically installs JavaScript dependencies for repositories with `package.json`; that installer allows network access and consumes repository-controlled lockfile URLs. A malicious repository can therefore make onboarding contact attacker-chosen or internal hosts. Skip dependency installation during evidence collection or restrict its network destinations.
Inspect repositories without installing dependencies, or restrict dependency installation to explicitly approved hosts.
</issue_to_address>
### Comment 6
<location path="a2a_bridge.py" line_range="28-31" />
<code_context>
+ "http://localhost:18001/api/automation/v1/events/"
+ "46d09c5c-7d7d-585f-8c37-a6b45a3de738/hermes-test"
+)
+API_BASE = "http://localhost:18001/api/automation"
+AUTOMATION_ID = "5e872472-b4cb-43f0-a3f4-30b57fcc4203"
+ORG_ID = "46d09c5c-7d7d-585f-8c37-a6b45a3de738"
+AGENT_SERVER_URL = "http://localhost:18000"
+
+WEBHOOK_SECRET = os.environ.get("A2A_WEBHOOK_SECRET", "")
</code_context>
<issue_to_address>
**Session key sent over HTTP**
When an untrusted local process can receive traffic on either configured port, `poll_run_status` and `read_agent_answer` send `A2A_SESSION_KEY` to localhost services over plaintext HTTP; a local process that captures the configured port can read the key and reuse it to access the API. Use an authenticated encrypted transport or otherwise ensure the local endpoint cannot be impersonated.
Use TLS with endpoint verification for these requests, or restrict the service and client to an authenticated local transport that prevents port impersonation.
</issue_to_address>
### Comment 7
<location path="onboard.py" line_range="940" />
<code_context>
+ lines.append("## What this repo is")
+ lines.append("")
+ if ev.readme_summary:
+ lines.append(ev.readme_summary)
+ elif info.description:
+ lines.append(info.description)
</code_context>
<issue_to_address>
**Repository text becomes agent instructions**
When a target repository's README contains instructions aimed at agents and an agent later consumes the generated `CLAUDE.md`, `onboard_repo` copies the target README summary directly into generated `CLAUDE.md`; a malicious repository owner can place prompt-injection instructions in that prose, which downstream coding agents then consume as project guidance. Treat the copied text as untrusted or keep it out of instruction-bearing content.
Keep repository-authored prose explicitly isolated and labeled as untrusted reference material rather than embedding it as agent guidance.
</issue_to_address>
### Comment 8
<location path="onboard.py" line_range="821" />
<code_context>
+ match = re.match(r"^(npm|pnpm|yarn|bun)@", str(declared))
+ if match:
+ ev.package_manager = match.group(1)
+ ev.generated.add("package_manager")
+
+ # package.json scripts (per sub-project)
</code_context>
<issue_to_address>
**Empty checkouts pass evidence gate**
When `--require-evidence` is used on an empty or metadata-free checkout, `_collect_evidence` unconditionally adds `package_manager` to `generated`, so `Evidence.detected` is true even when no repository facts were found. Onboarding writes boilerplate instead of refusing the checkout.
Add `package_manager` to `generated` only when evidence supports it, and ensure `detected` reflects actual repository facts.
</issue_to_address>
### Comment 9
<location path="onboard.py" line_range="1495" />
<code_context>
+ written.append(str(out_root / L1_FILE))
+
+ (out_root / PROJECT_VERIFY).write_text(
+ generate_project_verify(ev, info, stamp, sha), encoding="utf-8")
+ written.append(str(out_root / PROJECT_VERIFY))
+
</code_context>
<issue_to_address>
**Custom verification settings disappear**
When the target already has hand-maintained verification commands or envit settings, `write_artifacts` overwrites `project_verify.yaml` and envit files with generated content. Custom checks or curated settings that the generator does not rediscover are lost.
Preserve existing hand-maintained settings when writing generated verification and envit files.
Also at `onboard.py:1517-1520`.
</issue_to_address>
### Comment 10
<location path="onboard.py" line_range="1515-1531" />
<code_context>
+ written.append(str(agents_path))
+
+ if ev.dependency_repos:
+ manifest = build_envit_manifest(ev, info)
+ (out_root / ENVIT_MANIFEST).write_text(_json_dump(manifest), encoding="utf-8")
+ written.append(str(out_root / ENVIT_MANIFEST))
+ lock = build_envit_lock(ev, info, out_root)
+ (out_root / ENVIT_LOCK).write_text(_json_dump(lock), encoding="utf-8")
+ written.append(str(out_root / ENVIT_LOCK))
+ repos_root = envit_dir / "repos"
+ repos_root.mkdir(parents=True, exist_ok=True)
+ for dep in ev.envit_entries:
+ link = repos_root / dep["name"]
+ if link.is_symlink() or link.exists():
+ continue
+ store = _envit_store_path(dep)
+ if store is not None:
+ link.symlink_to(store, target_is_directory=True)
+ written.append(str(link))
+
+ return written
+
</code_context>
<issue_to_address>
**Removed dependencies remain declared**
When a later onboarding run finds no dependencies, or previously declared dependencies have been removed, `write_artifacts` skips writing empty envit declarations and does not remove obsolete declarations or repo symlinks. Existing files therefore continue to describe dependencies that the current evidence no longer finds.
Write empty envit declarations when there are no dependencies and remove obsolete generated declarations and symlinks.
</issue_to_address>
### Comment 11
<location path="onboard.py" line_range="1379-1378" />
<code_context>
+#!/usr/bin/env python3
+"""A2A bridge: dispatch → poll → read for OpenHands automation webhook.
+
+Stdlib only. CLI:
+ python3 a2a_bridge.py --task "<text>"
</code_context>
<issue_to_address>
**Generated checks lack required tools**
When the target declares scripts or Python checks whose tools or dependencies are not provisioned in the hermetic verification shell, `generate_project_verify` emits commands that the shell cannot run because their tools or dependencies are unavailable. The verify gate reports infrastructure failures instead of checking the target.
Only emit checks whose tools and dependencies are provisioned, or provision them in the verification shell.
Also at `onboard.py:1376-1378`, `onboard.py:1390-1394`.
</issue_to_address>
### Comment 12
<location path="onboard.py" line_range="185" />
<code_context>
+ "yaml": [
+ {"name": "yaml-validate",
+ "cmd": "python3 -c \"import yaml, sys; [yaml.safe_load(open(f)) for f in sys.argv[1:]]\" "
+ "$(find . -name '*.yaml' -o -name '*.yml' | grep -v node_modules | head -5) 2>&1 || true",
+ "cwd": ".", "languages": "yaml"},
+ ],
</code_context>
<issue_to_address>
**Malformed files pass validation**
When a generated YAML or JSON validator selects a malformed file, the generated validation commands end in `|| true`, converting parse failures to success, so `verify_gate` reports invalid files as passing checks.
Remove the success-forcing fallback so parser failures produce a failing check.
Also at `onboard.py:186`, `onboard.py:192`.
</issue_to_address>
### Comment 13
<location path="src/entire_review.py" line_range="202" />
<code_context>
try:
result = subprocess.run(
- ["entire", "review", "--base", base_branch, "--format", "text"],
+ [entire_path, "review", "--profile", "general", "--base", base_branch],
cwd=worktree_path,
capture_output=True,
</code_context>
<issue_to_address>
**Failed sensor appears to pass**
When the Entire CLI exits nonzero and its output contains no parseable findings, `run_entire_review` does not check the subprocess return code; when the CLI exits nonzero without parseable findings, the later severity calculation returns `status: "pass"` with `error: None`, so operators see a successful review despite the sensor failing.
Treat a nonzero return code or timeout as `status: "error"` and expose the failure instead of deriving a pass from an empty findings list.
</issue_to_address>
### Comment 14
<location path="a2a_bridge.py" line_range="103-104" />
<code_context>
+ # Route contract (router.py:104 prefix="/v1", router.py:690 route,
+ # mounted at app.py:300 under /api/automation) — NO "automations" segment.
+ url = (
+ f"{api_base}/v1/{automation_id}/runs?limit=5"
+ )
+ headers = {
+ "X-Session-API-Key": session_key,
</code_context>
<issue_to_address>
**Older runs are reported missing**
When more than five newer runs appear before the requested run in the list response, `poll_run_status` searches only the five runs returned by the list request and raises when the requested ID is absent. The bridge stops polling even though the run is still active or completed.
Use pagination or another lookup that can find the requested run beyond the five newest results.
</issue_to_address>
### Comment 15
<location path="onboard.py" line_range="1310-1317" />
<code_context>
+ }
+
+ # 2. git ls-remote (only for non-HEAD refs; HEAD resolves locally below)
+ url = source
+ if "github.com/" in source and not source.startswith(("http", "git@")):
+ url = f"https://github.com/{source}.git"
+ if url.startswith("http") or url.startswith("git@"):
+ want = ref if ref != "HEAD" else "HEAD"
+ spec = f"refs/tags/{want}" if (ref != "HEAD" and not _looks_like_sha(ref)) else want
+ for candidate in (spec, want):
+ out = _gh(["api", f"repos/{source}/commits/{candidate}"], timeout=20)
+ if out:
+ try:
</code_context>
<issue_to_address>
**Slug dependencies remain unpinned**
When a dependency uses the `owner/repo` source form with a non-SHA ref and no reusable lock entry, `_resolve_lock_entry` leaves `url` as the slug, so its URL-scheme guard skips the GitHub API lookup. The generated lock has a null commit and marks the dependency unresolved even when the repository is available.
Resolve `owner/repo` sources through the GitHub API as well as URL-form sources.
</issue_to_address>
### Comment 16
<location path="onboard.py" line_range="1317" />
<code_context>
+ want = ref if ref != "HEAD" else "HEAD"
+ spec = f"refs/tags/{want}" if (ref != "HEAD" and not _looks_like_sha(ref)) else want
+ for candidate in (spec, want):
+ out = _gh(["api", f"repos/{source}/commits/{candidate}"], timeout=20)
+ if out:
+ try:
</code_context>
<issue_to_address>
**Offline runs still contact GitHub**
When `--offline --write` is used with an unresolved dependency that has no reusable lock entry, `build_envit_lock` calls `_resolve_lock_entry`, which invokes `_gh` without honoring offline mode. Onboarding therefore makes GitHub requests and can incur repeated network timeouts.
Thread offline mode into lock resolution and avoid network lookups when it is enabled.
Also at `onboard.py:1594`.
</issue_to_address>
### Comment 17
<location path="onboard.py" line_range="1301-1302" />
<code_context>
+ except (OSError, json.JSONDecodeError):
+ continue
+ for locked in data.get("repos") or []:
+ if str(locked.get("name")) == name or locked.get("source", "").endswith(source):
+ return {
+ "name": name,
+ "source": str(locked.get("source") or source),
</code_context>
<issue_to_address>
**Wrong repository lock entry reused**
When a lockfile contains another repository with the same basename as the dependency, `_resolve_lock_entry` treats a matching basename as the same repository and can copy the other owner’s commit into this dependency’s lock entry. The generated lock then associates a commit with the wrong source.
Match reusable lock entries by the full repository source rather than basename alone.
Also at `onboard.py:1524-1525`.
</issue_to_address>
### Comment 18
<location path="onboard.py" line_range="684" />
<code_context>
+ slug = slug.rstrip("/")
+ if "/" not in slug or slug.count("/") != 1:
+ return
+ found.setdefault(slug, {"source": slug, "ref": ref, "via": via})
+
+ # 1a. pyproject git sources
</code_context>
<issue_to_address>
**Declared dependency pins are discarded**
When a pyproject GitHub dependency has an explicit uv `rev` or `tag`, the raw-text scan inserts the dependency with `ref="HEAD"` before the structured uv-source scan reads its explicit revision or tag. `_add` keeps the first entry, so the generated manifest and lock can lose the declared pin.
Give structured dependency declarations precedence over raw-text discoveries, or update an existing entry when a more specific ref is found.
Also at `onboard.py:691-703`.
</issue_to_address>
### Comment 19
<location path="onboard.py" line_range="1466" />
<code_context>
+ re.S,
+ )
+ if pattern.search(text):
+ return pattern.sub(new_block, text, count=1)
+ return text.rstrip("\n") + "\n\n" + new_block
+
</code_context>
<issue_to_address>
**Generated headings accumulate**
When onboarding writes generated files a second time or later, `_replace_generated_block` replaces only the fenced section, but its replacement includes content before the marker, such as the document heading. Each rerun leaves the old heading and inserts another.
Replace only the generated section, or pass a replacement that excludes content before its marker.
Also at `onboard.py:1486`, `onboard.py:1491`, `onboard.py:1510`.
</issue_to_address>
### Comment 20
<location path="onboard.py" line_range="1466" />
<code_context>
+ re.S,
+ )
+ if pattern.search(text):
+ return pattern.sub(new_block, text, count=1)
+ return text.rstrip("\n") + "\n\n" + new_block
+
</code_context>
<issue_to_address>
**Backslashes break generated refresh**
When a rerun replaces a generated block whose repo-derived content contains a backslash escape or group-reference sequence, `re.sub` interprets `new_block` as a replacement template, so a backslash sequence from repo-derived text is parsed as a replacement escape or group reference. A generated README summary containing such a sequence raises an exception on refresh instead of updating the file.
Pass a callable replacement such as `lambda _: new_block` so the generated text is inserted literally.
</issue_to_address>
### Comment 21
<location path="onboard.py" line_range="179" />
<code_context>
+ "languages": "typescript"},
+ ],
+ "javascript": [
+ {"name": "node-check", "cmd": "node --check \"$(find . -name '*.js' -not -path './node_modules/*' | head -1)\" 2>&1",
+ "cwd": ".", "languages": "javascript"},
+ ],
</code_context>
<issue_to_address>
**JavaScript fallback misses source files**
When a repo has `.mjs` or `.jsx` files but no `.js` file and no declared gate command, the JavaScript fallback searches only for `*.js`, so its filename substitution is empty for these repos and `node --check` fails without checking the source.
Include the detected JavaScript extensions in the fallback file search.
Also at `onboard.py:109`, `onboard.py:180`.
</issue_to_address>
### Comment 22
<location path="onboard.py" line_range="1675" />
<code_context>
+
+
+def main(argv: Optional[list[str]] = None) -> int:
+ parser = argparse.ArgumentParser(
+ prog="onboard",
+ description="Generate ICM L0-L4 context + envit + project_verify for a target repo.",
</code_context>
<issue_to_address>
**Documented dry-run flag is rejected**
When a caller runs the documented `--dry-run` command, the documented `onboard --dry-run` invocation reaches `argparse` without a registered `--dry-run` option and exits with a usage error, so callers cannot use the advertised command.
Register `--dry-run` as an alias for the default report-only behavior or remove it from the usage documentation.
</issue_to_address>
### Comment 23
<location path="onboard.py" line_range="1579" />
<code_context>
+ sha = short_sha()
+
+ local = Path(repo).expanduser()
+ if local.exists() and (local / ".git").exists():
+ repo_path = local.resolve()
+ slug = _remote_slug(repo_path) or repo
</code_context>
<issue_to_address>
**Local worktrees are rejected**
When `--repo` points to a Git worktree and `--no-clone` is used, or cloning is otherwise selected, `onboard_repo` recognizes a local checkout only when `.git` is a directory. In a Git worktree `.git` is a file, so `--no-clone` rejects the checkout and the default clone path treats the local path as a GitHub slug; onboarding a valid local worktree fails.
Recognize a checkout by running `git -C <path> rev-parse --show-toplevel` or by accepting either a `.git` file or directory.
</issue_to_address>
### Comment 24
<location path="a2a_bridge.py" line_range="157" />
<code_context>
+ if isinstance(content, list):
+ for part in content:
+ if part.get("type") == "text":
+ return part.get("text")
+ return None
+
</code_context>
<issue_to_address>
**Assistant answers lose later text**
When the newest assistant event contains more than one text part, `extract_last_assistant_text` returns on the first text part of the newest assistant event, so when that event contains multiple text parts the bridge prints only the first and truncates the answer.
Collect and join all text parts from the newest assistant event before returning the answer.
</issue_to_address>
### Comment 25
<location path="tests/test_a2a_bridge.py" line_range="168" />
<code_context>
+#!/usr/bin/env python3
+"""A2A bridge: dispatch → poll → read for OpenHands automation webhook.
+
+Stdlib only. CLI:
+ python3 a2a_bridge.py --task "<text>"
</code_context>
<issue_to_address>
**Wrong run can pass polling test**
When A poll response contains a terminal run with an ID different from the requested run ID, `test_returns_terminal_run` checks status and conversation ID, not that the returned run has the requested ID. Since the fake response contains only the requested `run-1`, a poller that returns any terminal run—even when the requested ID is absent—still passes this test.
Add a response containing a different terminal run and assert that polling reports the requested run as missing rather than returning that run.
</issue_to_address>
### Comment 26
<location path="onboard.py" line_range="691-692" />
<code_context>
+ text = _read_text(root / manifest)
+ if not text:
+ continue
+ for match in re.finditer(r"github\.com[/:]([A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+)", text):
+ _add(match.group(1), via=f"{manifest}")
+ if tomllib is not None:
+ try:
</code_context>
<issue_to_address>
**Metadata URLs become dependencies**
When a scanned file contains a GitHub URL that is not a dependency declaration, such as a project homepage URL or comment, the raw GitHub URL scan treats any matching URL in a `pyproject.toml` or requirements file as a dependency, even when it is only a repository/homepage link or comment. `_infer_dependencies` then writes that repo into the generated envit manifest as a dependency, polluting the target’s dependency rows with unrelated repos or itself.
Only add URLs parsed from dependency declarations and exclude the target repository itself.
Also at `onboard.py:710-712`.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 26 findings to address first, and the bridge never calls require_secrets, so it can send an empty-key HMAC and blank session header; if the local service accepts those credentials, the request can launch an automation run and its downstream actions. Reverting removes the bridge, but it would not undo runs or edits already triggered.
Blocking findings: issue_bridge.py:2713, conductor.py:664, a2a_bridge.py:215, onboard.py:516, onboard.py:1585, and 21 more
Comment on lines
+2694
to
+2713
| _entire_critical = [ | ||
| f for f in ((entire_review or {}).get("findings") or []) | ||
| if str((f or {}).get("severity", "")).upper() == "CRITICAL" | ||
| ] | ||
| if not _entire_critical and os.getenv("ENTIRE_HIGH_STRICT") == "1": | ||
| _entire_critical = [ | ||
| f for f in ((entire_review or {}).get("findings") or []) | ||
| if str((f or {}).get("severity", "")).upper() == "HIGH" | ||
| ] | ||
| if _entire_critical: | ||
| for f in _entire_critical: | ||
| adversarial_review.setdefault("findings", []).append({ | ||
| "section": "pre-merge-sensor", | ||
| "issue_class": "entire_finding", | ||
| "severity": "CRITICAL", | ||
| "citation": f"{f.get('file')}:{f.get('line')}", | ||
| "description": (f.get("message") or "")[:500], | ||
| "suggestion": "Fix the issue the pre-merge sensor flagged.", | ||
| }) | ||
| adversarial_review["verdict"] = "FAIL" |
There was a problem hiding this comment.
🟠 High · Critical findings do not reject runs
When a sync review has an authoritative canonical packet and Entire reports a CRITICAL finding or a strict HIGH finding, the sync path changes only adversarial_review, leaving the accepted canonical_packet unchanged. Later grading and publication use that packet, so the run continues despite the finding.
Update or invalidate the authoritative packet when the sync finding requires rejection, so downstream grading and publication reject the run.
Prompt for AI agents
In `issue_bridge.py` at lines 2694-2713:
**Critical findings do not reject runs**
When a sync review has an authoritative canonical packet and Entire reports a CRITICAL finding or a strict HIGH finding, the sync path changes only `adversarial_review`, leaving the accepted `canonical_packet` unchanged. Later grading and publication use that packet, so the run continues despite the finding.
Update or invalidate the authoritative packet when the sync finding requires rejection, so downstream grading and publication reject the run.
Comment on lines
+658
to
+664
| locked_update_bookbag( | ||
| leaf.bead, | ||
| repo=target_repo or "__global__", | ||
| lock_timeout=10.0, | ||
| entire_findings=entire_findings, | ||
| entire_status=entire_status, | ||
| ) |
There was a problem hiding this comment.
🟠 High · Acceptance misses sensor findings
When the bookbag lock cannot be acquired within the timeout while the Entire result is being persisted, locked_update_bookbag returns None without raising, and the caller proceeds to _persist_acceptance with the Entire findings unwritten. The acceptance gate can therefore accept work with a CRITICAL finding.
Check the update result and prevent acceptance from proceeding unless the Entire findings were persisted or otherwise included in the gate.
Prompt for AI agents
In `conductor.py` at lines 658-664:
**Acceptance misses sensor findings**
When the bookbag lock cannot be acquired within the timeout while the Entire result is being persisted, `locked_update_bookbag` returns `None` without raising, and the caller proceeds to `_persist_acceptance` with the Entire findings unwritten. The acceptance gate can therefore accept work with a CRITICAL finding.
Check the update result and prevent acceptance from proceeding unless the Entire findings were persisted or otherwise included in the gate.| parser.error("--task is required for full pipeline") | ||
|
|
||
| print(f"Dispatching task: {args.task}", file=sys.stderr) | ||
| resp = dispatch(WEBHOOK_URL, WEBHOOK_SECRET, args.task) |
There was a problem hiding this comment.
🟡 Medium · Missing credentials reach requests
When either A2A_WEBHOOK_SECRET or A2A_SESSION_KEY is unset, main does not call require_secrets before dispatch or polling, so the module-level empty values reach the requests. The bridge reports an authentication failure instead of the documented configuration error, and an endpoint accepting empty credentials receives unauthenticated requests.
Call require_secrets before dispatching or polling.
Also at a2a_bridge.py:33-34, a2a_bridge.py:216, a2a_bridge.py:226, a2a_bridge.py:248-249.
Prompt for AI agents
In `a2a_bridge.py` at line 215:
**Missing credentials reach requests**
When either `A2A_WEBHOOK_SECRET` or `A2A_SESSION_KEY` is unset, `main` does not call `require_secrets` before dispatch or polling, so the module-level empty values reach the requests. The bridge reports an authentication failure instead of the documented configuration error, and an endpoint accepting empty credentials receives unauthenticated requests.
Call `require_secrets` before dispatching or polling.
Also at `a2a_bridge.py:33-34`, `a2a_bridge.py:216`, `a2a_bridge.py:226`, `a2a_bridge.py:248-249`.| readme = "" | ||
| for name in ("README.md", "README.rst", "README.txt", "readme.md"): | ||
| if name in files: | ||
| readme = _read_text(root / name) |
There was a problem hiding this comment.
🔴 Critical · Repository symlinks escape checkout
When an onboarded repository contains a symlink at a read or generated-output path and the operator runs onboarding, a malicious checkout can make _read_text follow a tracked README symlink to a host file and copy its contents into generated artifacts; symlinks at output paths such as CLAUDE.md also make write_text overwrite files outside the checkout. Reject symlinks and verify resolved paths stay under the intended root before reading or writing.
Reject symlinks and ensure each resolved input and output path remains inside its intended checkout or output root.
Also at onboard.py:1486.
Prompt for AI agents
In `onboard.py` at line 516:
**Repository symlinks escape checkout**
When an onboarded repository contains a symlink at a read or generated-output path and the operator runs onboarding, a malicious checkout can make `_read_text` follow a tracked README symlink to a host file and copy its contents into generated artifacts; symlinks at output paths such as `CLAUDE.md` also make `write_text` overwrite files outside the checkout. Reject symlinks and verify resolved paths stay under the intended root before reading or writing.
Reject symlinks and ensure each resolved input and output path remains inside its intended checkout or output root.
Also at `onboard.py:1486`.| elif clone: | ||
| from src.core.repo_reader import clone_repo | ||
| repo_path = clone_repo(repo) | ||
| slug = repo |
There was a problem hiding this comment.
🟡 Medium · Onboarding triggers arbitrary requests
When the selected repository has a JavaScript manifest and a lockfile referencing an attacker-controlled or internal URL, onboard_repo calls clone_repo, which automatically installs JavaScript dependencies for repositories with package.json; that installer allows network access and consumes repository-controlled lockfile URLs. A malicious repository can therefore make onboarding contact attacker-chosen or internal hosts. Skip dependency installation during evidence collection or restrict its network destinations.
Inspect repositories without installing dependencies, or restrict dependency installation to explicitly approved hosts.
Prompt for AI agents
In `onboard.py` at line 1585:
**Onboarding triggers arbitrary requests**
When the selected repository has a JavaScript manifest and a lockfile referencing an attacker-controlled or internal URL, `onboard_repo` calls `clone_repo`, which automatically installs JavaScript dependencies for repositories with `package.json`; that installer allows network access and consumes repository-controlled lockfile URLs. A malicious repository can therefore make onboarding contact attacker-chosen or internal hosts. Skip dependency installation during evidence collection or restrict its network destinations.
Inspect repositories without installing dependencies, or restrict dependency installation to explicitly approved hosts.|
|
||
|
|
||
| def main(argv: Optional[list[str]] = None) -> int: | ||
| parser = argparse.ArgumentParser( |
There was a problem hiding this comment.
🟡 Medium · Documented dry-run flag is rejected
When a caller runs the documented --dry-run command, the documented onboard --dry-run invocation reaches argparse without a registered --dry-run option and exits with a usage error, so callers cannot use the advertised command.
Register --dry-run as an alias for the default report-only behavior or remove it from the usage documentation.
Prompt for AI agents
In `onboard.py` at line 1675:
**Documented dry-run flag is rejected**
When a caller runs the documented `--dry-run` command, the documented `onboard --dry-run` invocation reaches `argparse` without a registered `--dry-run` option and exits with a usage error, so callers cannot use the advertised command.
Register `--dry-run` as an alias for the default report-only behavior or remove it from the usage documentation.| sha = short_sha() | ||
|
|
||
| local = Path(repo).expanduser() | ||
| if local.exists() and (local / ".git").exists(): |
There was a problem hiding this comment.
🟡 Medium · Local worktrees are rejected
When --repo points to a Git worktree and --no-clone is used, or cloning is otherwise selected, onboard_repo recognizes a local checkout only when .git is a directory. In a Git worktree .git is a file, so --no-clone rejects the checkout and the default clone path treats the local path as a GitHub slug; onboarding a valid local worktree fails.
Recognize a checkout by running git -C <path> rev-parse --show-toplevel or by accepting either a .git file or directory.
Prompt for AI agents
In `onboard.py` at line 1579:
**Local worktrees are rejected**
When `--repo` points to a Git worktree and `--no-clone` is used, or cloning is otherwise selected, `onboard_repo` recognizes a local checkout only when `.git` is a directory. In a Git worktree `.git` is a file, so `--no-clone` rejects the checkout and the default clone path treats the local path as a GitHub slug; onboarding a valid local worktree fails.
Recognize a checkout by running `git -C <path> rev-parse --show-toplevel` or by accepting either a `.git` file or directory.| if isinstance(content, list): | ||
| for part in content: | ||
| if part.get("type") == "text": | ||
| return part.get("text") |
There was a problem hiding this comment.
🟡 Medium · Assistant answers lose later text
When the newest assistant event contains more than one text part, extract_last_assistant_text returns on the first text part of the newest assistant event, so when that event contains multiple text parts the bridge prints only the first and truncates the answer.
Collect and join all text parts from the newest assistant event before returning the answer.
Prompt for AI agents
In `a2a_bridge.py` at line 157:
**Assistant answers lose later text**
When the newest assistant event contains more than one text part, `extract_last_assistant_text` returns on the first text part of the newest assistant event, so when that event contains multiple text parts the bridge prints only the first and truncates the answer.
Collect and join all text parts from the newest assistant event before returning the answer.| ) | ||
| self.assertEqual(run["status"], "COMPLETED") | ||
| self.assertEqual(run["conversation_id"], "conv-1") | ||
|
|
There was a problem hiding this comment.
🟡 Medium · Wrong run can pass polling test
When A poll response contains a terminal run with an ID different from the requested run ID, test_returns_terminal_run checks status and conversation ID, not that the returned run has the requested ID. Since the fake response contains only the requested run-1, a poller that returns any terminal run—even when the requested ID is absent—still passes this test.
Add a response containing a different terminal run and assert that polling reports the requested run as missing rather than returning that run.
Prompt for AI agents
In `tests/test_a2a_bridge.py` at line 168:
**Wrong run can pass polling test**
When A poll response contains a terminal run with an ID different from the requested run ID, `test_returns_terminal_run` checks status and conversation ID, not that the returned run has the requested ID. Since the fake response contains only the requested `run-1`, a poller that returns any terminal run—even when the requested ID is absent—still passes this test.
Add a response containing a different terminal run and assert that polling reports the requested run as missing rather than returning that run.
Comment on lines
+691
to
+692
| for match in re.finditer(r"github\.com[/:]([A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+)", text): | ||
| _add(match.group(1), via=f"{manifest}") |
There was a problem hiding this comment.
🟡 Medium · Metadata URLs become dependencies
When a scanned file contains a GitHub URL that is not a dependency declaration, such as a project homepage URL or comment, the raw GitHub URL scan treats any matching URL in a pyproject.toml or requirements file as a dependency, even when it is only a repository/homepage link or comment. _infer_dependencies then writes that repo into the generated envit manifest as a dependency, polluting the target’s dependency rows with unrelated repos or itself.
Only add URLs parsed from dependency declarations and exclude the target repository itself.
Also at onboard.py:710-712.
Prompt for AI agents
In `onboard.py` at lines 691-692:
**Metadata URLs become dependencies**
When a scanned file contains a GitHub URL that is not a dependency declaration, such as a project homepage URL or comment, the raw GitHub URL scan treats any matching URL in a `pyproject.toml` or requirements file as a dependency, even when it is only a repository/homepage link or comment. `_infer_dependencies` then writes that repo into the generated envit manifest as a dependency, polluting the target’s dependency rows with unrelated repos or itself.
Only add URLs parsed from dependency declarations and exclude the target repository itself.
Also at `onboard.py:710-712`.![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+1494
to
+1495
| (out_root / PROJECT_VERIFY).write_text( | ||
| generate_project_verify(ev, info, stamp, sha), encoding="utf-8") |
There was a problem hiding this comment.
If a target checkout contains a project_verify.yaml symlink to a writable file outside the checkout, --write follows it and replaces that file with generated YAML. Reject symlinks and paths that resolve outside the chosen output directory before writing any artifacts, including paths beneath .envit.
How this was verified: The default output is the target checkout, and the artifact writes follow its paths without checking where they resolve.
Prompt To Fix With AI
This is a comment left during a code review.
Path: onboard.py
Line: 1494-1495
Comment:
**Repo links overwrite outside files**
If a target checkout contains a `project_verify.yaml` symlink to a writable file outside the checkout, `--write` follows it and replaces that file with generated YAML. Reject symlinks and paths that resolve outside the chosen output directory before writing any artifacts, including paths beneath `.envit`.
**How this was verified:** The default output is the target checkout, and the artifact writes follow its paths without checking where they resolve.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+1494
to
+1495
| (out_root / PROJECT_VERIFY).write_text( | ||
| generate_project_verify(ev, info, stamp, sha), encoding="utf-8") |
There was a problem hiding this comment.
--write replaces existing project_verify.yaml and envit.json files instead of preserving their hand-written settings. Custom verify commands disappear, and dependency skill paths and picks become generated defaults. This contradicts the documented promise that reruns preserve hand-written content. Merge the generated settings into existing files, or require a separate overwrite option.
Prompt To Fix With AI
This is a comment left during a code review.
Path: onboard.py
Line: 1494-1495
Comment:
**Custom settings disappear on refresh**
`--write` replaces existing `project_verify.yaml` and `envit.json` files instead of preserving their hand-written settings. Custom verify commands disappear, and dependency skill paths and picks become generated defaults. This contradicts the documented promise that reruns preserve hand-written content. Merge the generated settings into existing files, or require a separate overwrite option.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+1389
to
+1392
| if "pytest" in text or (ev.root / cwd / "tests").is_dir(): | ||
| _add(f"{cwd}:pytest", "pytest -q", cwd, ["python"]) | ||
| if "[tool.ruff]" in text: | ||
| _add(f"{cwd}:ruff", "ruff check .", cwd, ["python"]) |
There was a problem hiding this comment.
generate_project_verify emits pytest and ruff commands, but school-core's fixed verifyShell provides neither tool. The normal bridge path filters their “command not found” failures as infrastructure noise. These commands also suppress the Python compile fallback because they already claim the python language. Provision the tools and their dependencies, or retain a Python check the shell can run.
Prompt To Fix With AI
This is a comment left during a code review.
Path: onboard.py
Line: 1389-1392
Comment:
**Generated Python checks cannot run**
`generate_project_verify` emits `pytest` and `ruff` commands, but school-core's fixed `verifyShell` provides neither tool. The normal bridge path filters their “command not found” failures as infrastructure noise. These commands also suppress the Python compile fallback because they already claim the `python` language. Provision the tools and their dependencies, or retain a Python check the shell can run.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| re.S, | ||
| ) | ||
| if pattern.search(text): | ||
| return pattern.sub(new_block, text, count=1) |
There was a problem hiding this comment.
pattern.sub reads new_block as a replacement template, not literal text. The block includes README prose and raw package.json commands. If either contains \d or \s, the second onboarding run fails with a bad-escape error. Other escapes can silently change the output. Use a callable replacement to preserve the generated text exactly.
Suggested change
| return pattern.sub(new_block, text, count=1) | |
| return pattern.sub(lambda _: new_block, text, count=1) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: onboard.py
Line: 1466
Comment:
**Backslashes break context refreshes**
`pattern.sub` reads `new_block` as a replacement template, not literal text. The block includes README prose and raw `package.json` commands. If either contains `\d` or `\s`, the second onboarding run fails with a bad-escape error. Other escapes can silently change the output. Use a callable replacement to preserve the generated text exactly.
```suggestion
return pattern.sub(lambda _: new_block, text, count=1)
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+102
to
+104
| url = ( | ||
| f"{api_base}/v1/{automation_id}/runs?limit=5" | ||
| ) |
There was a problem hiding this comment.
poll_run_status searches only five runs and immediately raises if the requested ID is absent. An older run requested with --poll-only or --read-only, or a running task displaced by newer runs, can exist outside that page. Fetch the requested run directly or search later pages before declaring it missing.
Prompt To Fix With AI
This is a comment left during a code review.
Path: a2a_bridge.py
Line: 102-104
Comment:
**Existing runs appear missing**
`poll_run_status` searches only five runs and immediately raises if the requested ID is absent. An older run requested with `--poll-only` or `--read-only`, or a running task displaced by newer runs, can exist outside that page. Fetch the requested run directly or search later pages before declaring it missing.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+230
to
+232
| if run.get("status") == "FAILED": | ||
| print(f"Run failed: {run.get('error_detail', 'unknown')}", file=sys.stderr) | ||
| return 1 |
There was a problem hiding this comment.
poll_run_status returns CANCELLED runs, but the full pipeline rejects only FAILED. A cancelled run can therefore print an earlier assistant message and exit successfully, making unfinished work look complete. Require COMPLETED before reading a successful answer and return a failure for cancellation.
Suggested change
| if run.get("status") == "FAILED": | |
| print(f"Run failed: {run.get('error_detail', 'unknown')}", file=sys.stderr) | |
| return 1 | |
| if run.get("status") != "COMPLETED": | |
| print( | |
| f"Run {run.get('status')}: {run.get('error_detail', 'unknown')}", | |
| file=sys.stderr, | |
| ) | |
| return 1 |
Prompt To Fix With AI
This is a comment left during a code review.
Path: a2a_bridge.py
Line: 230-232
Comment:
**Cancelled tasks report success**
`poll_run_status` returns `CANCELLED` runs, but the full pipeline rejects only `FAILED`. A cancelled run can therefore print an earlier assistant message and exit successfully, making unfinished work look complete. Require `COMPLETED` before reading a successful answer and return a failure for cancellation.
```suggestion
if run.get("status") != "COMPLETED":
print(
f"Run {run.get('status')}: {run.get('error_detail', 'unknown')}",
file=sys.stderr,
)
return 1
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+155
to
+157
| for part in content: | ||
| if part.get("type") == "text": | ||
| return part.get("text") |
There was a problem hiding this comment.
extract_last_assistant_text returns from inside the content loop, so a message with multiple text parts loses everything after the first part. Join all text parts from the latest assistant message before returning, and add a multipart test.
Prompt To Fix With AI
This is a comment left during a code review.
Path: a2a_bridge.py
Line: 155-157
Comment:
**Answers lose later text parts**
`extract_last_assistant_text` returns from inside the content loop, so a message with multiple text parts loses everything after the first part. Join all text parts from the latest assistant message before returning, and add a multipart test.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+1461
to
+1466
| pattern = re.compile( | ||
| r"<!-- BEGIN ONBOARD-GENERATED:.*?(?:<!-- END ONBOARD-GENERATED -->\n?)", | ||
| re.S, | ||
| ) | ||
| if pattern.search(text): | ||
| return pattern.sub(new_block, text, count=1) |
There was a problem hiding this comment.
The generators put each title before BEGIN ONBOARD-GENERATED. This replacement leaves the old title in place, then inserts the entire new document with another title. Every refresh adds one more heading. Put generated titles inside the fence or replace only the fenced portion of the new document, and test several reruns for unchanged title counts.
Prompt To Fix With AI
This is a comment left during a code review.
Path: onboard.py
Line: 1461-1466
Comment:
**Refreshes duplicate document titles**
The generators put each title before `BEGIN ONBOARD-GENERATED`. This replacement leaves the old title in place, then inserts the entire new document with another title. Every refresh adds one more heading. Put generated titles inside the fence or replace only the fenced portion of the new document, and test several reruns for unchanged title counts.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+214
to
+215
| print(f"Dispatching task: {args.task}", file=sys.stderr) | ||
| resp = dispatch(WEBHOOK_URL, WEBHOOK_SECRET, args.task) |
There was a problem hiding this comment.
main never calls require_secrets, so the CLI sends requests with empty credentials when setup is incomplete. Users get a remote request error instead of being told which credentials are missing. Call the check before making requests and add a CLI test that confirms missing credentials stop before dispatch.
Prompt To Fix With AI
This is a comment left during a code review.
Path: a2a_bridge.py
Line: 214-215
Comment:
**Missing credentials reach the server**
`main` never calls `require_secrets`, so the CLI sends requests with empty credentials when setup is incomplete. Users get a remote request error instead of being told which credentials are missing. Call the check before making requests and add a CLI test that confirms missing credentials stop before dispatch.
---
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.
Hey - I've found 1 security issue, and 21 other issues
Security issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="issue_bridge.py" line_range="2713-2714" />
<code_context>
+ "description": (f.get("message") or "")[:500],
+ "suggestion": "Fix the issue the pre-merge sensor flagged.",
+ })
+ adversarial_review["verdict"] = "FAIL"
+ adversarial_review["score"] = 0.0
+
# Merge verify-gate failures into the review as CRITICAL findings.
</code_context>
<issue_to_address>
**Critical findings do not veto publication**
When the sync path receives a CRITICAL Entire finding for an accepted authoritative packet, `adversarial_review` changes to FAIL, but `canonical_packet.accepted` remains true and the later grading and PR-publication path continues, so work with a critical finding can still be graded or published as accepted.
Apply the Entire veto to the authoritative packet or stop downstream grading and publication when the finding is veto-worthy.
</issue_to_address>
### Comment 2
<location path="src/entire_review.py" line_range="202" />
<code_context>
try:
result = subprocess.run(
- ["entire", "review", "--base", base_branch, "--format", "text"],
+ [entire_path, "review", "--profile", "general", "--base", base_branch],
cwd=worktree_path,
capture_output=True,
</code_context>
<issue_to_address>
**Failed Entire reviews pass**
When the Entire subprocess exits nonzero, times out, or raises and its output has no parsed findings, `run_entire_review` derives status from parsed findings without treating the subprocess failure as a sensor error, so it returns `status: pass` and callers see a clean review.
Return a failed review status and error when the Entire subprocess does not complete successfully.
</issue_to_address>
### Comment 3
<location path="onboard.py" line_range="185" />
<code_context>
+ "yaml": [
+ {"name": "yaml-validate",
+ "cmd": "python3 -c \"import yaml, sys; [yaml.safe_load(open(f)) for f in sys.argv[1:]]\" "
+ "$(find . -name '*.yaml' -o -name '*.yml' | grep -v node_modules | head -5) 2>&1 || true",
+ "cwd": ".", "languages": "yaml"},
+ ],
</code_context>
<issue_to_address>
**Malformed files pass validation**
When a generated YAML, JSON, or Markdown validation command encounters invalid input, the generated validation command ends with `|| true`, converting parser failures or unreadable files into successful checks, so the verify gate reports malformed files as passing.
Remove the success-forcing fallback and preserve validator failures as a nonzero command result.
Also at `onboard.py:186`, `onboard.py:191-192`, `onboard.py:198`.
</issue_to_address>
### Comment 4
<location path="onboard.py" line_range="1301-1302" />
<code_context>
+ except (OSError, json.JSONDecodeError):
+ continue
+ for locked in data.get("repos") or []:
+ if str(locked.get("name")) == name or locked.get("source", "").endswith(source):
+ return {
+ "name": name,
+ "source": str(locked.get("source") or source),
</code_context>
<issue_to_address>
**Same-name dependencies resolve incorrectly**
When distinct dependency repositories from different owners share a basename, `_resolve_lock_entry` can reuse a lock entry based only on the basename, and materialized links also use that basename; the generated lock or context can therefore point at the wrong repository.
Match lock entries by full repository source and use distinct link paths for repositories with the same basename.
Also at `onboard.py:1503-1505`, `onboard.py:1524-1530`.
</issue_to_address>
### Comment 5
<location path="onboard.py" line_range="1310-1313" />
<code_context>
+ }
+
+ # 2. git ls-remote (only for non-HEAD refs; HEAD resolves locally below)
+ url = source
+ if "github.com/" in source and not source.startswith(("http", "git@")):
+ url = f"https://github.com/{source}.git"
+ if url.startswith("http") or url.startswith("git@"):
+ want = ref if ref != "HEAD" else "HEAD"
+ spec = f"refs/tags/{want}" if (ref != "HEAD" and not _looks_like_sha(ref)) else want
</code_context>
<issue_to_address>
**Dependency locks lack resolved commits**
When a dependency uses the `owner/repo` source form, has no reusable lock entry, and its ref is not a SHA, `_resolve_lock_entry` leaves the slug unchanged, so its URL check skips the GitHub lookup and writes `commit: null`; the generated lock does not pin the dependency.
Normalize `owner/repo` sources to a GitHub URL before the lookup, or perform the lookup directly from the slug.
</issue_to_address>
### Comment 6
<location path="onboard.py" line_range="1547-1549" />
<code_context>
+ base = Path.home() / ".envit" / "store" / "checkouts" / "github.com" / owner / name
+ if not base.is_dir():
+ return None
+ for child in sorted(base.iterdir()):
+ if child.is_dir() and not child.name.endswith(".git"):
+ return child
+ return None
+
</code_context>
<issue_to_address>
**Dependency links expose the wrong revision**
When the envit store has multiple checkouts or an existing link targets a checkout different from the lock commit, `_envit_store_path` selects the first checkout without checking its commit, and the writer can leave an existing link untouched; the generated context can expose a revision other than the one declared in the lock.
Select or refresh each dependency link only after verifying that its checkout matches the generated lock commit.
Also at `onboard.py:1524-1530`.
</issue_to_address>
### Comment 7
<location path="conductor.py" line_range="658-664" />
<code_context>
+ # CRITICAL entire finding could not veto acceptance. Write them here,
+ # repo-namespaced to match read_bookbag/_persist_acceptance.
+ try:
+ locked_update_bookbag(
+ leaf.bead,
+ repo=target_repo or "__global__",
+ lock_timeout=10.0,
+ entire_findings=entire_findings,
+ entire_status=entire_status,
+ )
+ except Exception as e: # noqa: BLE001
+ print(f" ⚠ Entire review result persist failed: {e}")
</code_context>
<issue_to_address>
**Unlocked teacher write erases sensor result**
When the teacher reads the bookbag before the sensor update and saves its copy afterward, the conductor's locked sensor update can race with the teacher's `update_bookbag`: the teacher reads the bag before the sensor write, then writes its stale copy afterward and drops `entire_findings` and `entire_status`. Acceptance then reads a bag without the finding and can pass it.
Make all bookbag writers use the same lock-protected update path so the sensor fields cannot be lost.
</issue_to_address>
### Comment 8
<location path="onboard.py" line_range="1317" />
<code_context>
+ want = ref if ref != "HEAD" else "HEAD"
+ spec = f"refs/tags/{want}" if (ref != "HEAD" and not _looks_like_sha(ref)) else want
+ for candidate in (spec, want):
+ out = _gh(["api", f"repos/{source}/commits/{candidate}"], timeout=20)
+ if out:
+ try:
</code_context>
<issue_to_address>
**Offline onboarding still calls GitHub**
When `--offline --write` is used for a repo with dependencies that have no reusable lock entries, `--offline` affects metadata lookup but is not passed to `build_envit_lock`; writing artifacts can therefore call `_gh` to resolve dependency commits. An offline run with dependencies and no reusable lock still attempts GitHub API requests and emits unresolved locks if those requests fail.
Propagate offline mode to lock resolution and skip all GitHub lookups when it is enabled.
Also at `onboard.py:1519-1520`.
</issue_to_address>
### Comment 9
<location path="a2a_bridge.py" line_range="215" />
<code_context>
+ parser.error("--task is required for full pipeline")
+
+ print(f"Dispatching task: {args.task}", file=sys.stderr)
+ resp = dispatch(WEBHOOK_URL, WEBHOOK_SECRET, args.task)
+ print(f"Dispatch response: {resp}", file=sys.stderr)
+
</code_context>
<issue_to_address>
**Missing credentials fail remotely**
When either required A2A environment variable is unset, `main` does not call `require_secrets`, so `dispatch` and later requests use empty credentials and fail at the service request instead of reporting the missing configuration clearly.
Call `require_secrets` before dispatching or making requests.
</issue_to_address>
### Comment 10
<location path="onboard.py" line_range="691-684" />
<code_context>
+ text = _read_text(root / manifest)
+ if not text:
+ continue
+ for match in re.finditer(r"github\.com[/:]([A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+)", text):
+ _add(match.group(1), via=f"{manifest}")
+ if tomllib is not None:
</code_context>
<issue_to_address>
**Explicit dependency pins are discarded**
When a pyproject GitHub dependency has an explicit uv `rev` or `tag`, the raw-text scan inserts the dependency with `ref="HEAD"` first, and the later parsed entry cannot replace it because `_add` uses `setdefault`; the generated manifest and lock lose the declared pin.
Let the parsed uv source replace the raw-text entry when it provides an explicit `rev` or `tag`.
Also at `onboard.py:684-685`, `onboard.py:692-703`.
</issue_to_address>
### Comment 11
<location path="onboard.py" line_range="1515-1518" />
<code_context>
+ written.append(str(agents_path))
+
+ if ev.dependency_repos:
+ manifest = build_envit_manifest(ev, info)
+ (out_root / ENVIT_MANIFEST).write_text(_json_dump(manifest), encoding="utf-8")
+ written.append(str(out_root / ENVIT_MANIFEST))
+ lock = build_envit_lock(ev, info, out_root)
+ (out_root / ENVIT_LOCK).write_text(_json_dump(lock), encoding="utf-8")
+ written.append(str(out_root / ENVIT_LOCK))
</code_context>
<issue_to_address>
**Removed dependencies remain declared**
When a later onboarding run discovers no dependencies after dependencies were previously declared, `write_artifacts` skips rewriting the envit files when there are no dependencies, leaving the old manifest and lock in place; the generated context and durable dependency declaration disagree.
Write empty or refreshed envit files and remove stale dependency links when no dependencies remain.
Also at `onboard.py:1524-1530`.
</issue_to_address>
### Comment 12
<location path="onboard.py" line_range="1516-1519" />
<code_context>
+
+ if ev.dependency_repos:
+ manifest = build_envit_manifest(ev, info)
+ (out_root / ENVIT_MANIFEST).write_text(_json_dump(manifest), encoding="utf-8")
+ written.append(str(out_root / ENVIT_MANIFEST))
+ lock = build_envit_lock(ev, info, out_root)
+ (out_root / ENVIT_LOCK).write_text(_json_dump(lock), encoding="utf-8")
+ written.append(str(out_root / ENVIT_LOCK))
+ repos_root = envit_dir / "repos"
</code_context>
<issue_to_address>
**Manifest and lock can diverge**
When onboarding stops or lock generation or writing fails after the manifest is written, `write_artifacts` writes the manifest before completing the lockfile, leaving a new declaration alongside an old or missing lock; later consumers can use inconsistent dependency state.
Stage the manifest and lock together and replace the existing files only after both are ready.
</issue_to_address>
### Comment 13
<location path="onboard.py" line_range="822" />
<code_context>
+#!/usr/bin/env python3
+"""A2A bridge: dispatch → poll → read for OpenHands automation webhook.
+
+Stdlib only. CLI:
+ python3 a2a_bridge.py --task "<text>"
</code_context>
<issue_to_address>
**Empty checkouts pass evidence checks**
When `--require-evidence` is used on a checkout with no concrete repo-derived facts, `_collect_evidence` unconditionally adds `package_manager` to `generated`, making `Evidence.detected` true and allowing boilerplate generation without evidence.
Add the package-manager marker only when repository evidence supports it.
Also at `onboard.py:821`.
</issue_to_address>
### Comment 14
<location path="onboard.py" line_range="1486" />
<code_context>
+
+ claude = generate_claude_md(info, ev, stamp, sha)
+ (out_root / L0_FILE).write_text(
+ _replace_generated_block(_read_text(out_root / L0_FILE), claude), encoding="utf-8")
+ written.append(str(out_root / L0_FILE))
+
</code_context>
<issue_to_address>
**Generated documents gain duplicate titles**
When onboarding writes `CLAUDE.md` or `CONTEXT.md` more than once, the generated title sits before the replacement marker, while the replacement block includes another title; reruns preserve the old title and add duplicate document content.
Place the title inside the replaced block or exclude it from the replacement content.
Also at `onboard.py:914`, `onboard.py:1466`, `onboard.py:1491`.
</issue_to_address>
### Comment 15
<location path="a2a_bridge.py" line_range="230-231" />
<code_context>
+ )
+ print(f"Run status: {run.get('status')}", file=sys.stderr)
+
+ if run.get("status") == "FAILED":
+ print(f"Run failed: {run.get('error_detail', 'unknown')}", file=sys.stderr)
+ return 1
+
</code_context>
<issue_to_address>
**Cancelled runs can return answers**
When a cancelled run has a conversation with assistant text, when a run is `CANCELLED` but has a conversation containing assistant text, the full pipeline checks only for `FAILED`, and `--read-only` checks no terminal status at all. Both paths read and print that conversation as a successful answer.
Require `COMPLETED` before reading or returning an answer, and return a failure for `FAILED` or `CANCELLED` in both modes.
Also at `a2a_bridge.py:257-265`.
</issue_to_address>
### Comment 16
<location path="onboard.py" line_range="179-180" />
<code_context>
+ "languages": "typescript"},
+ ],
+ "javascript": [
+ {"name": "node-check", "cmd": "node --check \"$(find . -name '*.js' -not -path './node_modules/*' | head -1)\" 2>&1",
+ "cwd": ".", "languages": "javascript"},
+ ],
+ "yaml": [
</code_context>
<issue_to_address>
**JavaScript fallback misses modules**
When a target has JavaScript evidence only in `.mjs` files and no project-specific verify command, when a repo uses `.mjs` files but has no `.js` file, `_LANG_VERIFY` still emits the JavaScript fallback, but its `find` searches only for `*.js`. The generated check runs with no source file and fails instead of validating the repo's JavaScript.
Search the JavaScript extensions the evidence collector recognizes, and handle the case where no matching source file exists.
</issue_to_address>
### Comment 17
<location path="onboard.py" line_range="1495-1496" />
<code_context>
+ written.append(str(out_root / L1_FILE))
+
+ (out_root / PROJECT_VERIFY).write_text(
+ generate_project_verify(ev, info, stamp, sha), encoding="utf-8")
+ written.append(str(out_root / PROJECT_VERIFY))
+
+ if not write_envit:
</code_context>
<issue_to_address>
**Onboarding overwrites existing verify commands**
When the target already has hand-maintained verify entries not reproduced by the generator, `write_artifacts` writes the generated command list directly to `project_verify.yaml` instead of preserving or merging existing entries, so commands maintained by the target repo but not rediscovered by onboarding disappear from the verify gate.
Preserve user-owned entries or require an explicit overwrite option before replacing an existing verify manifest.
</issue_to_address>
### Comment 18
<location path="onboard.py" line_range="1486-1487" />
<code_context>
+
+ claude = generate_claude_md(info, ev, stamp, sha)
+ (out_root / L0_FILE).write_text(
+ _replace_generated_block(_read_text(out_root / L0_FILE), claude), encoding="utf-8")
+ written.append(str(out_root / L0_FILE))
+
+ context = generate_context_md(info, ev, stamp, sha)
</code_context>
<issue_to_address>
**Symlinks overwrite outside files**
When an operator runs `onboard --write` on a checkout containing a symlink at a generated-artifact path, `write_artifacts` calls `Path.write_text` on fixed artifact paths without checking whether an existing path is a symlink. A target repository author can include a symlink such as `project_verify.yaml` pointing outside the checkout; when an operator runs onboarding with `--write`, the generated file follows that link and overwrites the external target.
Reject symlinked artifact paths or write through safely created temporary files using no-follow semantics.
</issue_to_address>
### Comment 19
<location path="onboard.py" line_range="940" />
<code_context>
+ lines.append("## What this repo is")
+ lines.append("")
+ if ev.readme_summary:
+ lines.append(ev.readme_summary)
+ elif info.description:
+ lines.append(info.description)
</code_context>
<issue_to_address>
**README injects agent instructions**
When a target repository's README summary contains instructions aimed at agents and an agent later consumes the generated `CLAUDE.md`, `generate_claude_md` inserts the repository-controlled README summary as ordinary text in the agent-facing `CLAUDE.md`. A repository author can place model-directed instructions in that summary, which downstream agents may treat as trusted project guidance and follow during work.
Clearly delimit repository-derived prose as untrusted reference material and instruct consuming agents not to treat it as policy or commands.
</issue_to_address>
### Comment 20
<location path="tests/test_onboard.py" line_range="515" />
<code_context>
+ empty = tmp_path / "empty"
+ empty.mkdir()
+ with pytest.raises(OnboardError):
+ onboard_repo(str(empty), write=True, clone=False, require_evidence=True)
+
+ def test_cli_report_only_is_exit_zero(self, tmp_path, capsys):
</code_context>
<issue_to_address>
**Evidence guard is bypassed**
When the empty directory has no `.git` entry and cloning is disabled, `test_requires_evidence_refuses_empty_repo` passes at lines 514–515 because `onboard_repo` rejects the directory as not a checkout before reaching the evidence check; removing `require_evidence` enforcement would still leave this test green.
Make the fixture an empty git checkout so the call reaches the evidence check.
</issue_to_address>
### Comment 21
<location path="tests/test_a2a_bridge.py" line_range="131" />
<code_context>
+ def test_posts_signed_body_and_parses_response(self):
+ with FakeServer() as base:
+ resp = bridge.dispatch(f"{base}/dispatch", "test-secret", "do a thing")
+ self.assertTrue(resp["received"])
+ self.assertEqual(resp["runs_created"], ["run-1"])
+
</code_context>
<issue_to_address>
**A2A request inputs go unchecked**
When the bridge posts wrong task JSON or requests a different conversation ID, `test_posts_signed_body_and_parses_response` (lines 128–132) and `test_reads_agent_answer` (258–261) still pass: POST checks only a signature over any bytes, and GET returns the answer for any `/events` path.
Assert the decoded task body and the exact conversation-specific request path.
Also at `tests/test_a2a_bridge.py:261`.
</issue_to_address>
### Comment 22
<location path="src/entire_review.py" line_range="201-205" />
<code_context>
result = subprocess.run(
[entire_path, "review", "--profile", "general", "--base", base_branch],
cwd=worktree_path,
capture_output=True,
text=True,
timeout=120,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>Sourcery assessment
Approval pending. 22 findings to address first.
Blocking findings: issue_bridge.py:2714, src/entire_review.py:202, onboard.py:185, onboard.py:1302, onboard.py:1313, and 17 more
Comment on lines
+2713
to
+2714
| adversarial_review["verdict"] = "FAIL" | ||
| adversarial_review["score"] = 0.0 |
There was a problem hiding this comment.
🟠 High · Critical findings do not veto publication
When the sync path receives a CRITICAL Entire finding for an accepted authoritative packet, adversarial_review changes to FAIL, but canonical_packet.accepted remains true and the later grading and PR-publication path continues, so work with a critical finding can still be graded or published as accepted.
Apply the Entire veto to the authoritative packet or stop downstream grading and publication when the finding is veto-worthy.
Prompt for AI agents
In `issue_bridge.py` at lines 2713-2714:
**Critical findings do not veto publication**
When the sync path receives a CRITICAL Entire finding for an accepted authoritative packet, `adversarial_review` changes to FAIL, but `canonical_packet.accepted` remains true and the later grading and PR-publication path continues, so work with a critical finding can still be graded or published as accepted.
Apply the Entire veto to the authoritative packet or stop downstream grading and publication when the finding is veto-worthy.| try: | ||
| result = subprocess.run( | ||
| ["entire", "review", "--base", base_branch, "--format", "text"], | ||
| [entire_path, "review", "--profile", "general", "--base", base_branch], |
There was a problem hiding this comment.
🟡 Medium · Failed Entire reviews pass
When the Entire subprocess exits nonzero, times out, or raises and its output has no parsed findings, run_entire_review derives status from parsed findings without treating the subprocess failure as a sensor error, so it returns status: pass and callers see a clean review.
Return a failed review status and error when the Entire subprocess does not complete successfully.
Prompt for AI agents
In `src/entire_review.py` at line 202:
**Failed Entire reviews pass**
When the Entire subprocess exits nonzero, times out, or raises and its output has no parsed findings, `run_entire_review` derives status from parsed findings without treating the subprocess failure as a sensor error, so it returns `status: pass` and callers see a clean review.
Return a failed review status and error when the Entire subprocess does not complete successfully.| "yaml": [ | ||
| {"name": "yaml-validate", | ||
| "cmd": "python3 -c \"import yaml, sys; [yaml.safe_load(open(f)) for f in sys.argv[1:]]\" " | ||
| "$(find . -name '*.yaml' -o -name '*.yml' | grep -v node_modules | head -5) 2>&1 || true", |
There was a problem hiding this comment.
🟡 Medium · Malformed files pass validation
When a generated YAML, JSON, or Markdown validation command encounters invalid input, the generated validation command ends with || true, converting parser failures or unreadable files into successful checks, so the verify gate reports malformed files as passing.
Remove the success-forcing fallback and preserve validator failures as a nonzero command result.
Also at onboard.py:186, onboard.py:191-192, onboard.py:198.
Prompt for AI agents
In `onboard.py` at line 185:
**Malformed files pass validation**
When a generated YAML, JSON, or Markdown validation command encounters invalid input, the generated validation command ends with `|| true`, converting parser failures or unreadable files into successful checks, so the verify gate reports malformed files as passing.
Remove the success-forcing fallback and preserve validator failures as a nonzero command result.
Also at `onboard.py:186`, `onboard.py:191-192`, `onboard.py:198`.
Comment on lines
+1301
to
+1302
| if str(locked.get("name")) == name or locked.get("source", "").endswith(source): | ||
| return { |
There was a problem hiding this comment.
🟠 High · Same-name dependencies resolve incorrectly
When distinct dependency repositories from different owners share a basename, _resolve_lock_entry can reuse a lock entry based only on the basename, and materialized links also use that basename; the generated lock or context can therefore point at the wrong repository.
Match lock entries by full repository source and use distinct link paths for repositories with the same basename.
Also at onboard.py:1503-1505, onboard.py:1524-1530.
Prompt for AI agents
In `onboard.py` at lines 1301-1302:
**Same-name dependencies resolve incorrectly**
When distinct dependency repositories from different owners share a basename, `_resolve_lock_entry` can reuse a lock entry based only on the basename, and materialized links also use that basename; the generated lock or context can therefore point at the wrong repository.
Match lock entries by full repository source and use distinct link paths for repositories with the same basename.
Also at `onboard.py:1503-1505`, `onboard.py:1524-1530`.
Comment on lines
+1310
to
+1313
| url = source | ||
| if "github.com/" in source and not source.startswith(("http", "git@")): | ||
| url = f"https://github.com/{source}.git" | ||
| if url.startswith("http") or url.startswith("git@"): |
There was a problem hiding this comment.
🟠 High · Dependency locks lack resolved commits
When a dependency uses the owner/repo source form, has no reusable lock entry, and its ref is not a SHA, _resolve_lock_entry leaves the slug unchanged, so its URL check skips the GitHub lookup and writes commit: null; the generated lock does not pin the dependency.
Normalize owner/repo sources to a GitHub URL before the lookup, or perform the lookup directly from the slug.
Prompt for AI agents
In `onboard.py` at lines 1310-1313:
**Dependency locks lack resolved commits**
When a dependency uses the `owner/repo` source form, has no reusable lock entry, and its ref is not a SHA, `_resolve_lock_entry` leaves the slug unchanged, so its URL check skips the GitHub lookup and writes `commit: null`; the generated lock does not pin the dependency.
Normalize `owner/repo` sources to a GitHub URL before the lookup, or perform the lookup directly from the slug.
Comment on lines
+1486
to
+1487
| _replace_generated_block(_read_text(out_root / L0_FILE), claude), encoding="utf-8") | ||
| written.append(str(out_root / L0_FILE)) |
There was a problem hiding this comment.
🟡 Medium · Symlinks overwrite outside files
When an operator runs onboard --write on a checkout containing a symlink at a generated-artifact path, write_artifacts calls Path.write_text on fixed artifact paths without checking whether an existing path is a symlink. A target repository author can include a symlink such as project_verify.yaml pointing outside the checkout; when an operator runs onboarding with --write, the generated file follows that link and overwrites the external target.
Reject symlinked artifact paths or write through safely created temporary files using no-follow semantics.
Prompt for AI agents
In `onboard.py` at lines 1486-1487:
**Symlinks overwrite outside files**
When an operator runs `onboard --write` on a checkout containing a symlink at a generated-artifact path, `write_artifacts` calls `Path.write_text` on fixed artifact paths without checking whether an existing path is a symlink. A target repository author can include a symlink such as `project_verify.yaml` pointing outside the checkout; when an operator runs onboarding with `--write`, the generated file follows that link and overwrites the external target.
Reject symlinked artifact paths or write through safely created temporary files using no-follow semantics.| lines.append("## What this repo is") | ||
| lines.append("") | ||
| if ev.readme_summary: | ||
| lines.append(ev.readme_summary) |
There was a problem hiding this comment.
🟡 Medium · README injects agent instructions
When a target repository's README summary contains instructions aimed at agents and an agent later consumes the generated CLAUDE.md, generate_claude_md inserts the repository-controlled README summary as ordinary text in the agent-facing CLAUDE.md. A repository author can place model-directed instructions in that summary, which downstream agents may treat as trusted project guidance and follow during work.
Clearly delimit repository-derived prose as untrusted reference material and instruct consuming agents not to treat it as policy or commands.
Prompt for AI agents
In `onboard.py` at line 940:
**README injects agent instructions**
When a target repository's README summary contains instructions aimed at agents and an agent later consumes the generated `CLAUDE.md`, `generate_claude_md` inserts the repository-controlled README summary as ordinary text in the agent-facing `CLAUDE.md`. A repository author can place model-directed instructions in that summary, which downstream agents may treat as trusted project guidance and follow during work.
Clearly delimit repository-derived prose as untrusted reference material and instruct consuming agents not to treat it as policy or commands.| empty = tmp_path / "empty" | ||
| empty.mkdir() | ||
| with pytest.raises(OnboardError): | ||
| onboard_repo(str(empty), write=True, clone=False, require_evidence=True) |
There was a problem hiding this comment.
🟡 Medium · Evidence guard is bypassed
When the empty directory has no .git entry and cloning is disabled, test_requires_evidence_refuses_empty_repo passes at lines 514–515 because onboard_repo rejects the directory as not a checkout before reaching the evidence check; removing require_evidence enforcement would still leave this test green.
Make the fixture an empty git checkout so the call reaches the evidence check.
Prompt for AI agents
In `tests/test_onboard.py` at line 515:
**Evidence guard is bypassed**
When the empty directory has no `.git` entry and cloning is disabled, `test_requires_evidence_refuses_empty_repo` passes at lines 514–515 because `onboard_repo` rejects the directory as not a checkout before reaching the evidence check; removing `require_evidence` enforcement would still leave this test green.
Make the fixture an empty git checkout so the call reaches the evidence check.| def test_posts_signed_body_and_parses_response(self): | ||
| with FakeServer() as base: | ||
| resp = bridge.dispatch(f"{base}/dispatch", "test-secret", "do a thing") | ||
| self.assertTrue(resp["received"]) |
There was a problem hiding this comment.
🟡 Medium · A2A request inputs go unchecked
When the bridge posts wrong task JSON or requests a different conversation ID, test_posts_signed_body_and_parses_response (lines 128–132) and test_reads_agent_answer (258–261) still pass: POST checks only a signature over any bytes, and GET returns the answer for any /events path.
Assert the decoded task body and the exact conversation-specific request path.
Also at tests/test_a2a_bridge.py:261.
Prompt for AI agents
In `tests/test_a2a_bridge.py` at line 131:
**A2A request inputs go unchecked**
When the bridge posts wrong task JSON or requests a different conversation ID, `test_posts_signed_body_and_parses_response` (lines 128–132) and `test_reads_agent_answer` (258–261) still pass: POST checks only a signature over any bytes, and GET returns the answer for any `/events` path.
Assert the decoded task body and the exact conversation-specific request path.
Also at `tests/test_a2a_bridge.py:261`.
Comment on lines
201
to
205
| result = subprocess.run( | ||
| ["entire", "review", "--base", base_branch, "--format", "text"], | ||
| [entire_path, "review", "--profile", "general", "--base", base_branch], | ||
| cwd=worktree_path, | ||
| capture_output=True, | ||
| text=True, |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
This branch has not been deployed
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
2 participants
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.
What this is
This PR carries the behaviour changes that had been bundled into #152 (the 1nzr reorg). Splitting them out keeps #152 reorg-only.
school/1nzr-behaviour= #152's revert commitbb1b968+ a reapply commitd38dbfa. The tree is identical toschool/1nzr-reorgata2afda4739db1563a7eaa0874def21d10b8f3b52, so this PR's diff is exactly the behaviour that was removed from #152.Stacked on #152. Merge #152 first, then retarget this PR to
main.Behaviour changes carried here
onboard.py: new root module. It generates ICM L0-L4 context, envit entries andproject_verify.yamlfor a target repo. Tests:tests/test_onboard.py.issue_bridge.py--onboardCLI: passthrough flags (--onboard,--write,--out,--no-clone,--offline,--require-evidence) that dispatch toonboard.main().a2a_bridge.py: new root module. It sends tasks to OpenHands over A2A and retrieves their answers. Tests:tests/test_a2a_bridge.py. It is not wired into theissue_bridge.pyCLI on this branch.issue_bridge.pyinjects a CRITICAL entire finding intoadversarial_reviewand forcesverdict=FAILandscore=0.0. A HIGH finding does the same whenENTIRE_HIGH_STRICT=1. The sync-sensor test intests/test_issue_bridge.pynow uses a CRITICAL finding and expectsFAIL.conductor.pywritesentire_findingsandentire_statusto the bookbag withlocked_update_bookbag, before_persist_acceptancereads them.src/entire_review.py: renamesqodo_replacementtoentire_replacement. The CLI call changes from["entire", "review", "--base", base, "--format", "text"]to[entire_path, "review", "--profile", "general", "--base", base].review_packet.py::_sensor: normalizesfindingswhen it is a list rather than an int.docs/setup.md: new section "Onboarding a new target repo".docs/pipeline-explainer.md: Stage 7 rewritten to cover the CRITICAL veto,ENTIRE_HIGH_STRICTand--profile general.campus.md: Entire wording updated for the veto,--profile generaland the Qodo removal.project_verify.yaml: the compile-step comment namesonboardanda2a_bridgeagain. The command itself is the same as on 1nzr: reorganise flat root into src/{bridge,core,crew,review,sensors,tools} #152.Known open review findings (NOT fixed yet)
adversarial_review. It does not rejectcanonical_packetor stop grading and PR publication.conductor.pyignoreslocked_update_bookbagreturningNoneon lock timeout, so acceptance proceeds without the veto.onboard.py: the slug-vs-URL guard skips the online commit lookup forowner/reposources.onboard.py: existing lock entries are reused by basename alone.onboard.py: the raw-text scan'ssetdefaultwithref="HEAD"drops explicit[tool.uv.sources]pins.onboard.py:_envit_store_pathis not verified against the lock commit.onboard.py: the generated yaml/json validate checks end in|| true.onboard.py:_collect_evidencealways markspackage_manager, so--require-evidenceis a no-op.Workflow note
.github/workflows/ci.ymllistsonboard.py a2a_bridge.pyin the ruffCORElist. That matches this branch but not #152 after the split. It was not changed because the integration token lacksworkflowscope.Verification
No tests were run for this split.
Summary by Sourcery
Add repository onboarding and A2A automation while enforcing critical pre-merge findings across synchronous and asynchronous review paths.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
Do not merge until onboarding writes stay inside the checkout and the broken refresh, verification, and run-result paths are fixed.
Findings
Fix with agent prompt
Summary
Adds onboarding and a standalone OpenHands bridge. It also changes Entire review arguments, applies sync review vetoes, saves async findings before acceptance, and accepts list-shaped sensor findings.
Reviews (1) · Last reviewed commit: "feat(1nzr): reapply behaviour changes sp..." · Reviewed by Greptile