01a077f5 - Configure GitHub accounts and script-owned execution - #70
Conversation
|
Static-script validation for
This PR remains Draft. To proceed, restore the configured Grok account's usage balance and approve the fork workflow with an authorized maintainer account. The script can then run the required reviews and validate hosted CI on this head; no vendor substitution or readiness waiver was applied. This fork PR contains the complete change and supersedes the earlier documentation-only draft #69. That earlier PR has not been closed by the Jonny account. |
|
Static-script continuation for The fork workflow approval blocker is resolved. At this script's latest observation, hosted CI was: test: in_progress / pending. Both independent Grok review lanes were retried on this unchanged head and again returned HTTP 402: This PR remains Draft. The configured Grok account requires restored usage balance before the script can complete the prescribed Grok quality/logic reviews, then Codex quality/logic reviews. Hosted CI must also succeed on this head before Ready for review. No monitoring or waiting was delegated to a model. |
JonnyLuca
left a comment
JonnyLuca
left a comment
There was a problem hiding this comment.
Codex logic — rejected at 7d79e7e
src/agent_cli/github_accounts.py:85 — [P1] Git remotes containing embedded credentials or a separate pushurl bypass the selected gh credential helper. The executor can therefore push as another account despite verifying the configured login, violating account isolation; reject or sanitize such remotes before fetch/push.
src/agent_cli/main.py:2673 — [P2] measure_mergeable relies on the repository working directory, but with command_prefix the gh process runs in the executor’s default directory while cwd applies only to the host-side prefix. Container-backed accounts therefore cannot reliably locate the PR; route the mapped worktree directory into the executor or pass an explicit repository to gh.
JonnyLuca
left a comment
JonnyLuca
left a comment
There was a problem hiding this comment.
Codex quality — rejected at 7d79e7e
DESIGN.md:653: The claimed “no configuration means no GitHub execution” boundary is incomplete. agent a38 still invokes ambient gh repo view at src/agent_cli/a38.py:593 when --private is omitted, without loading github-accounts.json or a session binding. Scope the documentation accurately or route this reachable path through explicit account selection.
src/agent_cli/main.py:2673: Container-backed accounts cannot reliably perform the documented mergeable step. measure_mergeable invokes gh pr view without --repo, while Account.runner maps only Git’s -C path; the host cwd is not transferred through docker exec, so gh may fail or inspect the wrong repository. The container execution context needs a mapped working directory or explicit repository targeting, with coverage for this path.
|
Static-script correction update for The introduced review findings have been addressed in the proposed code: effective Git remote URL validation (including push URLs and Git rewrites), explicit repository/PR selection for container mergeability and fork targets, and precise documentation of the covered executors versus legacy ambient gh paths. Regression tests were added; eight incorrect test-double argv comparisons were subsequently corrected. Clean signed-head tests: All four independent PR reviews must pass again on this head. The static local launcher has restarted Grok quality/logic; Codex quality/logic follows only after both complete Grok approvals. Earlier approvals do not cover this changed head. Hosted CI for this head remains a separate requirement. The PR remains Draft. |
JonnyLuca
left a comment
JonnyLuca
left a comment
There was a problem hiding this comment.
Grok quality — rejected at c74b728
I'll review this change for quality only: first the review skill and CONTRIBUTING, then the patch and affected sources.Shell is blocked for this review, so I'll read the patch and affected files directly.Continuing through the rest of the patch and the affected source/tests.I'll inspect the key implementation points and docs against the patch for introduced quality defects.Checking one more parser edge case that could affect the claimed remote-validation guarantee:Quality review of HEAD c74b72821b31b75fbfadea33175d8dadbfd3dc71 against base 6877054590d9c27868826ef1805b5cd27c5c58ae is complete. Prior remote-isolation and docs-scope issues look addressed; one introduced defect remains in the new network-argv parser.
Finding
src/agent_cli/github_accounts.py:37:_FETCH_PUSH_OPTS_WITH_ARGincludes--recv-packbut not Git’s canonical--receive-pack; with_positionalstreating unknown flags as argument-less (github_accounts.py:255),git push --receive-pack <prog> <repository> …validates<prog>(or another decoy) and still executes the transfer to<repository>, so the effective-remote check documented indocs/github-accounts.md:60-70does not hold for a real push form. No test covers this shift.
Non-gating notes
- Fork-from-personal-repo vs
CONTRIBUTING.md: waived for this PR only, as authorized. - Ambient
ghonagent a38visibility lookup, and unimplemented AI accounts/roles: documented as remaining gaps, not claimed done here.
STATUS: complete
VERDICT: rejected
JonnyLuca
left a comment
JonnyLuca
left a comment
There was a problem hiding this comment.
Grok logic — rejected at c74b728
I'll review this change for logic defects only: first the review contract and CONTRIBUTING, then the patch and affected sources.Bash is blocked here, so I'll read the patch and affected sources with the file tools only.Continuing through the patch’s account-isolation and mergeability logic.I spotted a possible binding-persistence gap in scan_merged; next I'll verify that against the live sources and related call sites.Checking the network-option allowlist against real git option names for a validate-vs-execute mismatch.Logic review of c74b728 against 6877054 (account isolation, mergeability routing, retries, failure paths). Docs and AI/role gaps match the stated non-claims. Fork-PR waiver noted for same-repo-branch only.
Finding (introduced)
src/agent_cli/github_accounts.py:37-41: _FETCH_PUSH_OPTS_WITH_ARG lists --recv-pack but not git push’s real --receive-pack (and omits other space-separated options-with-args such as --refmap, --strategy, --onto).
src/agent_cli/github_accounts.py:247-256: _positionals treats an unknown long option as a boolean flag, so the next token becomes a “positional.”
src/agent_cli/github_accounts.py:335-339 + 394-407: _network_remote_targets returns only that first token for validation, then Account.runner still executes the original argv. Example: git push --receive-pack decoy origin HEAD:refs/heads/feature validates remote decoy and pushes to unvalidated origin. A worktree with a safe decoy remote and a credential-bearing or non-GitHub origin bypasses the effective-URL guard this change adds. --option=value forms are skipped safely; the hole is the space-separated form.
Inherited / out of gate (not blocking alone)
- Documented ambient
ghonagent a38visibility lookup; AI accounts/roles still unimplemented as stated. scan_mergedrefuses a changedexecution_accountbut does not pin one when absent; normalpr.openviascan_githubpins first.
STATUS: complete
VERDICT: rejected
|
Static-script correction update for The introduced review findings have been addressed in the proposed code: effective Git remote URL validation (including push URLs and Git rewrites), explicit repository/PR selection for container mergeability and fork targets, and precise documentation of the covered executors versus legacy ambient gh paths. Regression tests were added; eight incorrect test-double argv comparisons were subsequently corrected. Clean signed-head tests: All four independent PR reviews must pass again on this head. The static local launcher has restarted Grok quality/logic; Codex quality/logic follows only after both complete Grok approvals. Earlier approvals do not cover this changed head. Hosted CI for this head remains a separate requirement. The PR remains Draft. |
JonnyLuca
left a comment
JonnyLuca
left a comment
There was a problem hiding this comment.
Grok logic — rejected at 7025489
I'll review this PR for logic defects only: first the review contract and CONTRIBUTING, then the patch and affected code.Shell is blocked for this review, so I'll read the patch and affected files with the read tools only.Continuing through the patch, focusing on account isolation and Git/GitHub execution paths.Continuing the patch through the executor wiring and tests, and checking the HEAD sources for the critical logic paths.Inspecting multi-URL remote handling and related integration points for isolation gaps.Checking how account errors propagate in cmd_run and whether multi-URL remotes are handled consistently.
Logic review of 7025489c against 68770545 is complete. Checked CONTRIBUTING, the pr-review skill, the full patch, and the affected sources/tests (account runner, effective remote resolution, measure_mergeable, executor wiring, retries/bindings).
Introduced defect (gates):
src/agent_cli/github_accounts.py:211:resolve_effective_github_https_urlvalidates eachgit remote get-url --allURL only as HTTPS github.com shape, then returnsurls[0]; it never requires every URL in that list to resolve to the sameowner/name.validate_repo_remote(:218–:223) only compares the first fetch URL to the first push URL. A remote with an extra push URL (for examplegit remote set-url --add --push origin https://github.com/other/exfil.git) therefore passes the pre-transfer check while a latergit pushstill pushes to every push URL, including the second repository, with the selected account’s credentials. That breaks the remote-isolation guarantee this change introduces. Fix by requiring all URLs from each--alllist (and then fetch vs push) to share one case-insensitiveowner/namebefore allowing the network transfer.
Account isolation / retries / routing (no further introduced defects): Session binding is pinned on scan_github before execution and refused on change; existing PR reuse checks author login; missing/wrong auth does not fall back; container mergeability uses explicit --repo plus PR number or mapped git -C branch; require_git=False is limited to the known task PR path; failure paths fail closed without echoing URL secrets.
Inherited / documented (not gated): Ambient gh on agent a38 visibility lookup; AI accounts/roles remain requirements only, as stated.
STATUS: complete
VERDICT: rejected
|
Static-script correction update for The introduced review findings have been addressed in the proposed code: effective Git remote URL validation (including push URLs and Git rewrites), explicit repository/PR selection for container mergeability and fork targets, and precise documentation of the covered executors versus legacy ambient gh paths. Regression tests were added; eight incorrect test-double argv comparisons were subsequently corrected. Clean signed-head tests: All four independent PR reviews must pass again on this head. The static local launcher has restarted Grok quality/logic; Codex quality/logic follows only after both complete Grok approvals. Earlier approvals do not cover this changed head. Hosted CI for this head remains a separate requirement. The PR remains Draft. |
JonnyLuca
left a comment
JonnyLuca
left a comment
There was a problem hiding this comment.
Codex quality — rejected at cfb2e5f
src/agent_cli/github_accounts.py:180 git clone is treated and documented as supported, but explicit URLs are validated via git remote get-url in the destination’s current directory. That command requires an existing Git repository, so ordinary clones from a non-repository directory fail before cloning. Add a successful clone-path test and a repository-independent URL-resolution method.
src/agent_cli/github_accounts.py:375 Clearing only generic and host-wide http.extraHeader settings does not isolate Git authentication. A more-specific ambient http.https://github.com/<owner>/<repo>.extraHeader can still supply another account’s Authorization header, even though gh api user validated the configured account. This violates the documented no-fallback account boundary; neutralize URL-specific ambient authentication and test it.
STATUS: complete
VERDICT: rejected
|
Static-script correction update for The introduced review findings have been addressed in the proposed code: effective Git remote URL validation (including push URLs and Git rewrites), explicit repository/PR selection for container mergeability and fork targets, and precise documentation of the covered executors versus legacy ambient gh paths. Regression tests were added; eight incorrect test-double argv comparisons were subsequently corrected. Clean signed-head tests: All four independent PR reviews must pass again on this head. The static local launcher has restarted Grok quality/logic; Codex quality/logic follows only after both complete Grok approvals. Earlier approvals do not cover this changed head. Hosted CI for this head remains a separate requirement. The PR remains Draft. |
|
Maintainer action required for The full static-script test suite passed on this clean signed head: GitHub blocks this head's fork workflow with Earlier linked workflow runs belong to obsolete heads. The publishing account has read-only access to this target repository and cannot authorize the workflow itself. Authorization does not replace a successful CI result. The PR remains Draft. The additional formal APPROVE step in the packaged pr-review skill also remains unresolved because GitHub forbids author self-approval; no alternate account has been selected implicitly. No Ready transition or merge has occurred. |
JonnyLuca
left a comment
JonnyLuca
left a comment
There was a problem hiding this comment.
Codex logic — rejected at cb672c7
src/agent_cli/main.py:1185 — [P1] Requeuing an errored gate review replaces the entire activity with a fresh payload that omits execution_account. After a binding change, the retry therefore bypasses scan_github’s account-switch check; if the first account posted successfully but its response was lost, the second account can also post a duplicate review. Preserve the original binding when updating the errored activity.
|
Static-script correction update for The latest correction preserves the pinned execution account when an errored gate review is requeued, including recovery from a stale initial row read. Regression tests verify that changing the configured account still blocks execution before any GitHub call. Earlier corrections cover effective remote validation, non-repository clone resolution, URL-specific HTTP header isolation, explicit container/fork PR selection, and accurate implementation-boundary documentation. Clean signed-head tests: All four independent PR reviews must pass again on this head. The static local launcher has restarted Grok quality/logic; Codex quality/logic follows only after both complete Grok approvals. Earlier approvals do not cover this changed head. Hosted CI for this head remains a separate requirement. The PR remains Draft. |
|
Static-script review results for Review-pass count: 4. Independent Grok quality and logic passed completely, followed by independent Codex quality and logic; all four approved with zero introduced findings on this exact head. The review lanes performed bounded reviews only. Earlier head approvals were not reused. Full tests executed by the static script on the clean signed head: Current-head GitHub workflow inventory:
The PR remains Draft, not Ready for review or completed. An After CI succeeds, the script must still verify all readiness requirements. The packaged pr-review approval rule also asks for a formal GitHub APPROVE review; GitHub does not allow the PR author to approve their own PR. No alternate account has been selected implicitly. Human merge remains separate. |
marassteiner
left a comment
marassteiner
left a comment
There was a problem hiding this comment.
Formal approval for 534d71903fe0ee7e59effb81f148f68e94a02528, submitted by the static executor through the explicitly selected separate review account.
Evidence verified before submission:
- Four independent complete approvals on this exact head: Grok quality and logic, followed by Codex quality and logic; zero introduced findings.
- Full static-script tests passed on the clean signed head.
- Current-head GitHub workflow inventory and PR check rollup are green; PR is mergeable.
This publishes the recorded independent review results. It is not an additional model review and does not merge the PR. A human merges.
|
Ready for review evidence for
The earlier remote Grok balance and fork-CI approval blockers were resolved for this review by using the explicitly authorized local review launcher and the approved successful workflow. This is Ready for review, not merged or completed; merge remains with a human. |
3 participants
EN:
Add explicit GitHub execution accounts and session bindings with no default account or fallback identity. Document script ownership of assignment acceptance, lane starts, tests, GitHub communication and monitoring, and the required empty installation defaults. This Jonny-authored fork PR supersedes documentation-only draft #69.
DE:
Ergänzt explizite GitHub-Ausführungskonten und Session-Zuordnungen ohne Standardkonto oder Identitäts-Fallback. Dokumentiert die Script-Zuständigkeit für Übernahme, Lanes, Tests, GitHub und Monitoring sowie die geforderten leeren Installationsvorgaben. Dieser Fork-PR von Jonny ersetzt inhaltlich Dokumentationsentwurf #69.
Implementation scope and validation
GitHub configuration supports separate CLI credentials, Git signing identities, executor prefixes and worktree path mappings. Effective Git remote URLs are validated before supported transfers, including push URLs and Git URL rewrites; unsupported implicit or multi-remote forms are refused. Container mergeability uses explicit repository and PR selection, including fork targets. Explicit session bindings apply to the GitHub activity executor, assignment and merge scans, supervised issue reads, and pushed/mergeable steps.
Configurable AI accounts and user-defined roles remain documented requirements and are not implemented by this manifest. The end-to-end workflow still has explicitly documented implementation gaps; legacy ambient gh paths such as a38 visibility lookup remain outside this manifest, and no sandbox enforcement or deployment is claimed.
The user explicitly authorized this fork PR; the normal same-repository branch rule remains unchanged.
Validated head:
534d71903fe0ee7e59effb81f148f68e94a02528.Static-script repository tests:
1398 passed, 3 skipped, 47 subtests passed in 107.11s (0:01:47).Four independent complete review passes: Grok quality, Grok logic, then Codex quality and Codex logic; all approved with zero introduced findings. These reviews ran through the local static launcher after the remote Grok account returned a balance error.
GitHub Actions
testand the current-head check rollup are successful. The publication script verified the commit signature and a clean worktree.Ready for review is not merged or completed. A human merges.