impl: v2-phase-2 (b — lane) - #146
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f09898ddd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| --tools "Read,Write,Edit,Grep,Glob,Bash" | ||
| --allowedTools "Read,Write,Edit,Grep,Glob,Bash(${{ runner.temp }}/implement-publish.sh:*)" | ||
| --disallowedTools "Read(/.git/**),Write(/.git/**),Edit(/.git/**),Write(/.github/**),Edit(/.github/**),Write(/.claude/**),Edit(/.claude/**),Write(/AGENTS.md),Edit(/AGENTS.md),Write(/CLAUDE.md),Edit(/CLAUDE.md),Write(/REVIEW.md),Edit(/REVIEW.md),Write(/work/**/intent.md),Edit(/work/**/intent.md),Write(/work/**/spec.md),Edit(/work/**/spec.md),Bash(git push:*),Bash(gh pr create:*),Bash(gh pr edit:*),Bash(gh pr comment:*),Bash(gh api:*)" |
There was a problem hiding this comment.
The build action has repository-write credentials while granting the model unrestricted Write and Edit; the denylist protects selected repository paths but not files such as $HOME/.gitconfig. Artifact text can therefore make the model configure a Git hook or filter and edit an executable that the allowed publisher's git status or git commit runs, escaping the tool allowlist with the action token. Run this model step without write credentials, or run every Git command with isolated config and disabled hooks before a separate trusted publisher receives credentials.
AGENTS.md reference: AGENTS.md:L84-L90
Useful? React with 👍 / 👎.
| if [ "$EVENT_NAME" = workflow_run ] && [ "$base_sha" != "$EVENT_HEAD_SHA" ]; then | ||
| green="$(gh run list --workflow ci.yml --event push --commit "$base_sha" \ | ||
| --status success --limit 10 --json databaseId --jq 'length')" | ||
| if [ "$green" -eq 0 ]; then | ||
| echo "Main advanced to a push whose ci is not green yet; its completion will retry." |
There was a problem hiding this comment.
When main advances after this run's triggering merge but the newer commit's CI is still running, this exits and assumes that CI completion will retry the stage. If the newer commit did not touch a spec, however, its matching path-filtered implement-on-spec push run does not exist, so the later workflow_run is rejected by the “Confirm the matching path-filtered push run” step and the pending implementation remains stranded until a manual dispatch or another spec merge. The same race exists in spec-on-intent.yml; the retry must scan pending work after the newer CI succeeds or wait within the original run.
Useful? React with 👍 / 👎.
| github.event.pull_request.base.ref == github.event.repository.default_branch && | ||
| (github.actor == vars.YSTACK_OPERATOR || github.actor == 'claude[bot]') && | ||
| (github.triggering_actor == vars.YSTACK_OPERATOR || github.triggering_actor == 'claude[bot]') && | ||
| format('review-pr-{0}-{1}', github.event.pull_request.number, github.event.pull_request.head.sha) || |
There was a problem hiding this comment.
Including the head SHA in this concurrency key gives every synchronize event a different group, so cancel-in-progress cannot cancel the review of the previous head. That stale review can hold the global claude-quota slot until it eventually fails the moved-head check, delaying the review of the current commit and consuming an unnecessary model run. Key the cancellable group by PR number only; the posted marker can still record the reviewed head.
Useful? React with 👍 / 👎.
1 participant
PR B of
work/v2-phase-2/plan.md: the autonomous lane, two stage skills, and the three named docs.What changed
spec-on-intent.ymldrafts one hash-linked spec per pending intent. It is idempotent, draft-locks updates, refuses approved/stale PRs, and asserts the exact branch head and one-file diff.implement-on-spec.ymlcommits the plan first, commits code second, verifies the exact head in a credential-free no-network sandbox, then publishes through a separate command-only action.review-on-pr.ymlloads its workflow, skill, and policy from trusted main, snapshots the PR as data, gives the model one temporary output file, and posts/asserts one head/base-marked comment in a plain step./implementand/review-prcarry the stage boundaries, model policy, proof rules, and comments-only rail.work/README.mdnow describe both lanes and the setup needed for the autonomous one.Do not merge yet
This is a draft because the live repository does not yet meet the setup documented in QUICKSTART:
CLAUDE_CODE_OAUTH_TOKENfrom the repository secret to aystack-laneenvironment restricted tomain, then delete the repository-level copy.YSTACK_OPERATORrepository variable.ystack/spec/*andystack/impl/*from other writers.Two implementation choices also need the operator's explicit ruling because the accepted plan names a different event:
push. The path-filtered push run is therefore the quota record; the agent starts from the supportedworkflow_runevent after the same commit passesci.pull_requestworkflow executes PR-authored workflow code. Review therefore usespull_request_target, checks out only main, and reads the PR only through the API.The user's eight-file boundary also conflicts with the repo's full-backup rule: the five new load-bearing files are not yet in
ci/required-files.txt.RESTORE.mdstill calls the OAuth workflow optional, and v2 artifacts still carry no issue ID that generated PRs can use forCloses #…. Those need an operator ruling or a follow-up change before this can be called merge-ready.Verify evidence
verified-head: 3f09898ddd55ad6fc128e0954ffac17c1c9aadc0Unmasked actionlint reports only
queue: max; v1.7.12 predates that current GitHub syntax. No other workflow finding remains.New commits make this evidence stale.
Closes #144