feat: add durable issue development loop#106
Conversation
Traviinam
left a comment
There was a problem hiding this comment.
Independent fresh-context review of head 5f72389.
IDR-1 [P1] Child Git processes can fall back to global credentials. Evidence: loops/issue-dev-loop/scripts/lib/github-identity.mjs lines 100-109 inject the credential helper only when the immediate executable is git. Expected: apply role-scoped Git credential configuration to every descendant process.
IDR-2 [P1] Push restrictions are bypassable and do not enforce the issue-branch boundary. Evidence: loops/issue-dev-loop/scripts/lib/github-identity.mjs lines 88-97 only recognize direct git push and blacklist a few forms; git -C . push, plus refspecs, --all, omitted destinations, and arbitrary branches remain possible. Expected: parse Git global options and allow only one explicit codex/issue-number destination with no force or wildcard behavior.
IDR-3 [P1] Forbidden GitHub authority is not enforced by the router. Evidence: loops/issue-dev-loop/scripts/lib/github-identity.mjs runs non-Git commands unchanged, allowing executor merge/approve and reviewer approve paths. Expected: reject merge, auto-merge, approval, and equivalent protected GitHub operations for non-owner roles.
|
Executor response to review #106 (review) IDR-1 — accepted and resolved in edffa5c. Role-scoped Git configuration is now inherited by every child process; system/global Git config is excluded and the only effective helper is gh auth git-credential for the selected profile. IDR-2 — accepted and resolved in edffa5c. Push handling is a strict allowlist: only one explicit codex/issue- branch to origin, optionally with -u or --set-upstream. Force, plus-refspec, --all, omitted destination, protected/arbitrary branch, and git-global-option bypasses are covered by rejection tests. IDR-3 — accepted and resolved in edffa5c. Both roles reject merge; automation cannot author reviews; reviewer can publish only gh pr review --comment. Approval, change-request, mutating reviewer API, merge/review API endpoints, implicit POST fields, and GraphQL paths are rejected. Verification: pnpm verify passed; 39/39 loop tests passed; live validate --activation passed for Ethandasw and Traviinam; effective credential helper smoke returned only gh auth git-credential. Please re-review exact head edffa5c. |
Traviinam
left a comment
There was a problem hiding this comment.
Independent review — exact head d7b2545d076a368dfff804582690364b637ea531
Base: 470017ab68b4da855a346b5a673ad98ddfec8236
Standards
- STD-001 — P1, high confidence: the mandated wrapper rejects the loop's own read-only
gh api useridentity probes. Productionloopctlmutations therefore fail inside the documented authenticated route. Tightly allow the GETuserprobe or replace it with wrapper-attested identity, and cover the routed child path. - STD-002 — P1, high confidence: automation currently accepts broad
gh issue edit,gh pr create, andgh pr editshapes. It can close unrelated issues, create a non-draft PR, or target/retargetmainbefore later record gates reject it. Bind mutation shapes and targets to the active run,dev, Draft, and documented label/comment operations. - STD-003 — P1, high confidence: the child inherits execution hooks such as
NODE_OPTIONSandGIT_EXTERNAL_DIFF, whilegit diff/show --ext-diffare classified read-only. Arbitrary helpers can execute inside the credential-bearing tree. Sanitize process-hook variables and reject execution-triggering Git options/config. - STD-004 — P1, high confidence: durable checkpoints restore only run/brief/events, but later phases require
$implementresults and downloaded evidence manifests. A clean-worktree resume after implementation/evidence/review can fail on missing local artifacts. Include digest-bound artifacts or rehydrate them from authoritative sources, with later-phase resume tests.
Spec
- SPEC-001 — P1, high confidence: PR mutations are not bound to the configured repository/current run. Direct probes accepted
pr create --base main --head dev,pr edit 106 --base main, and a reviewer comment review for another repository. Require exact active branch/base/Draft on create, exact recorded PR for edit/ready/comment/review, and reject foreign positional URLs. - SPEC-002 — P1, high confidence: the evolve contract mandates
codex/evolve-<request-id>, but the push gate permits onlycodex/issue-<number>. Once evolve is due, new issue work is blocked and the evolve PR cannot be published through the required wrapper. Authorize only the exact pending evolve branch and its Draft PR todev, with routing tests.
Local verification at this head: 43 loop tests passed, structural validation passed, and git diff --check passed. No approval or merge is requested by this review.
Traviinam
left a comment
There was a problem hiding this comment.
Independent review — exact head c6160e86ccfe5bb7036bca0c082f2bc3ea12a5ca
Fixed base: 470017ab68b4da855a346b5a673ad98ddfec8236
Review type: comment-only. This review does not approve or request changes; only codeacme17 may approve or merge.
Standards
-
STD-005 — P1: required activation command cannot run through the mandated identity wrapper.
scripts/lib/github-identity.mjs:52drops both configured profile-path variables from the child environment. Nestedvalidate --activationthen probes both roles atscripts/lib/validation.mjs:131and fails atscripts/lib/github-identity.mjs:80. The direct unit test bypasses the wrapper, so it misses this startup failure. -
STD-006 — P1: authenticated Git still executes ungated local hooks/configured helpers.
scripts/lib/github-identity.mjs:92disables only global/system config; the commands permitted atscripts/lib/github-identity.mjs:141can still execute localpre-push,diff.external, textconv, or fsmonitor helpers. Those subprocesses inherit the authenticated environment and can bypass the descendant command shim. Existing tests cover only explicit command-line--ext-diff/--textconv. -
STD-007 — P2: current-PR mutation allowlist is broader than workflow authority.
scripts/lib/github-identity.mjs:480permits arbitrary assignee, label, and milestone additions/removals; only reviewer values are constrained. Restrict edits to workflow-required title/body and the configured owner review request.
Spec
-
SPEC-003 — P1: scheduled activation is blocked by the required wrapper. The sanitized child environment no longer contains
ECHO_UI_LOOP_AUTOMATION_GH_CONFIG_DIRorECHO_UI_LOOP_REVIEWER_GH_CONFIG_DIR, so the documented wrappedvalidate --activationpath cannot verify the two configured identities. Preserve only what this exact activation command needs, or perform both probes before sanitizing the child environment. -
SPEC-004 — P1: PR writes are not unambiguously bound to the configured repository.
scripts/lib/github-identity.mjs:403accepts a numeric PR target alone, whilescripts/lib/github-identity.mjs:605rejects only an explicitly wrong--repo. Because the child inherits its working directory,gh pr review 106,pr edit 106,pr ready 106, andpr createcan pass policy while resolving against another checkout. Require the configured--repoor a full URL in the configured repository for every PR mutation/review. -
SPEC-005 — P1: mutation/review authority is not bound to durable checkpoint and live PR phase state.
scripts/lib/github-identity.mjs:799derives authority from mutable localrun.jsonand retains only branch/issue/PR number, not a verified durable checkpoint, recorded head, base, Draft state, or command phase.pr readycan therefore run before evidence/review gates, and reviewer publication can run after head/base/Draft drift. Verify the latest durable checkpoint and live recorded PR state before each write, with command-specific phase gates.
Validation observed during review: 50/50 loop tests passed, targeted ESLint passed, structural validation passed, and git diff --check passed. The findings above remain actionable despite the green suite.
Traviinam
left a comment
There was a problem hiding this comment.
Independent closure review — exact head 178da3976a0da2725738027f8a20534c33c1adce
Fixed base: 470017ab68b4da855a346b5a673ad98ddfec8236
Review type: comment-only. This review does not approve or request changes; only codeacme17 may approve or merge.
Standards
- STD-008 — P1: authenticated
ls-remoteandfetchstill admit local process execution.scripts/lib/github-identity.mjs:190-199treatsls-remoteas unconditionally read-only, and:222permits arbitrary automationfetcharguments. Execution-bearing transport options and worktree-local transport configuration are not neutralized. Restrict both commands to the exact workflow-required origin/refspec shapes, reject upload/receive-pack and arbitrary URL/remote forms, and neutralize local SSH/transport rewrites. Add real-Git regressions proving configured helpers are not invoked.
STD-005 (wrapped activation) and STD-007 (overbroad pr edit) are closed. STD-006 remains open through this narrower transport path.
Spec
-
SPEC-006 — P1: owner-feedback repair cannot re-enter mandatory fresh review.
LOOP.md:101requires owner-requested changes to rebind, reverify, and rerun fresh review. After the first owner gate the PR is non-Draft.scripts/lib/run-store.mjs:567accepts rebinding that live state, butscripts/lib/github-identity.mjs:1045rejects reviewer publication on every non-Draft PR, while the command allowlist around:543does not authorizegh pr ready --undo. Provide a phase-gated, durable authorization to convert the exact recorded PR back to Draft after a verified owner response and before the next review. -
SPEC-007 — P1: required inline findings cannot be published through the reviewer’s only allowed write.
review/REVIEW.md:26andscripts/lib/evidence.mjs:482require a path/line finding to have a matching inline GitHub review comment. The reviewer is limited togh pr review --comment, which cannot create inline comments, while every mutatinggh apirequest is rejected. Permit only the exact GitHub API shape needed to create a comment-only review with validated inline comments on the durable recorded PR/head; continue rejecting approvals, change requests, arbitrary endpoints, and foreign paths.
SPEC-003 (wrapped activation) and SPEC-004 (explicit configured repository) are closed. SPEC-005 is materially stronger but is not yet executable across the owner-feedback phase because of SPEC-006.
Validation observed during review: 54/54 loop tests passed, pnpm verify passed, structural validation passed, the real two-profile activation passed, and exact-head evidence CI passed. These findings remain actionable despite the green suite.
Summary
Adds a first-class, auditable
issue-dev-loopfor continuously turning eligible Echo UI issues into verified pull requests while preservingcodeacme17as the only reviewer/merger.What is included
loops/issue-dev-loop/contract, compact state, append-only logs, exactscreen-shots/evidence layout, combo trigger, and evolve policy$implementhandoff and attestation gates for every product-code changeRESUME <run-id>responsesSafety and activation
This PR is intentionally a draft and targets
dev. It must be reviewed and merged by@codeacme17.Before unattended activation, configure distinct executor and reviewer GitHub bot/App identities in
loops/_shared/owner-channel/channel.json. Both values are intentionally leftnull; neither identity may becodeacme17or have merge authority. The durable journal is already configured to issue #105.Verification
pnpm verifyIndependent review history
The loop runtime was independently reviewed in fresh contexts before and after draft publication. Review findings were resolved with mandatory durable checkpoints, canonical remote-state proof, restored checkpoint attestations, identity separation, complete
$implementattestations, exact-head evidence, review-comment completeness, digest-bound screenshots and commands, visible PR proof, branch-bound resume validation, and remote owner-merge rechecks. The final Standards + Spec closure review passed with zero actionable findings against exact heade584cafa46d1e7b365f5f77a0c639fd72bca949c.