-
Notifications
You must be signed in to change notification settings - Fork 12
fix(#821): add commit-existence gate before declaring success #823
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -548,6 +548,14 @@ authoritative pre-commit check on the runner before pushing. | |
| 4. **Do not refactor to satisfy a linter.** Fix the specific reported | ||
| error — nothing more. | ||
|
|
||
| **Step sequence guardrail:** After step 9b completes — whether hooks | ||
| passed, failed, or encountered infrastructure errors — you MUST | ||
| continue to step 9c. After step 9c, continue to step 9d. After step | ||
| 9d, proceed to step 10 (commit). Do NOT skip steps 9c–11 regardless | ||
| of pre-commit outcome. Pre-commit failure is not a reason to stop or | ||
| to declare success — the post-script handles pre-commit | ||
| authoritatively on the runner. | ||
|
|
||
| **9c. Tests and linters — MANDATORY** | ||
|
|
||
| ```bash | ||
|
|
@@ -817,6 +825,34 @@ ran pre-commit in step 9b (which is the same check). Commit with | |
| `--no-verify` to bypass the git hook and disclose the failure in the commit | ||
| message. The post-script runs an authoritative pre-commit on the runner. | ||
|
|
||
| **Commit-existence verification:** After committing (and after gitlint | ||
| validation, if applicable), verify that at least one commit exists on | ||
| the feature branch: | ||
|
|
||
| ```bash | ||
| git log --oneline <target-branch>..HEAD | ||
| ``` | ||
|
|
||
| Use the local `<target-branch>` ref discovered in step 3. If the | ||
| command returns no output (zero commits), something went wrong — the | ||
| commit was not created. Check for uncommitted changes: | ||
|
|
||
| ```bash | ||
| git status --porcelain | ||
| ``` | ||
|
|
||
| - **If uncommitted changes exist:** Go back to step 10a and stage and | ||
| commit them. This can happen if `git commit` failed silently (e.g., | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] edge-case Prior finding resolved: the commit-existence verification recovery path now includes a one-retry bound ('If the commit still does not appear after one retry, report the failure in structured output and stop.'), consistent with the bounded-retry pattern used elsewhere. |
||
| a hook rejection) or if you forgot to run `git commit`. If the | ||
| commit still does not appear after one retry, report the failure in | ||
| structured output and stop. | ||
| - **If no uncommitted changes and no commits:** Your implementation | ||
| was lost. Report this clearly in the structured output and stop. | ||
|
|
||
| Do NOT proceed to step 10d or step 11 without at least one commit on | ||
| the feature branch. This check prevents the agent from declaring | ||
| success without committing — the root cause of silent work loss. | ||
|
|
||
| **Do not push the branch.** The post-script handles pushing, PR creation, | ||
| and failure reporting. | ||
|
|
||
|
|
@@ -874,11 +910,28 @@ checks `$FULLSEND_OUTPUT_DIR/agent-result.json` against | |
| `schemas/code-result.schema.json`. If validation fails, the harness | ||
| retries the agent. Producing a valid output file is not optional. | ||
|
|
||
| ```bash | ||
| echo "::notice::STEP 11: Validate structured output" | ||
| ``` | ||
|
|
||
| **Commit-existence check:** Before finalizing, verify that you | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] naming-convention The callout 'Commit-existence check:' (line 917) uses different terminology than the identical block at line 828 ('Commit-existence verification:'), though both perform the same git log operation for the same purpose. Consider standardizing to one term. |
||
| actually committed your work. If you made code changes, `git log` | ||
| must show at least one commit on the feature branch: | ||
|
|
||
| ```bash | ||
| git log --oneline <target-branch>..HEAD | ||
| ``` | ||
|
|
||
| If you made implementation changes but this shows zero commits, you | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [medium] logic-error The step 11 commit-existence check contradicts the step 9c retry-limit failure path. When tests/linters fail after exhausting retries, step 9c explicitly instructs: 'do not commit. Validate structured output, then stop.' The agent reaches step 11 without a commit — by design. However, the new step 11 check says: 'If you made implementation changes but this shows zero commits, you MUST go back to step 10 and commit before continuing.' This could force the agent to commit code that failed tests, directly contradicting the retry-limit instruction. The ambiguity is compounded by the new step sequence guardrail ('Do NOT skip steps 9c-11'), which — while scoped to pre-commit outcome — could be interpreted more broadly by an LLM agent. Suggested fix: Add a qualifying condition, e.g.: 'If you made implementation changes, tests passed, and this shows zero commits, you MUST go back to step 10 and commit before continuing.' |
||
| MUST go back to step 10 and commit before continuing. Do NOT declare | ||
| success without a commit. Do NOT write `agent-result.json` and exit. | ||
| Staged-but-uncommitted changes are silently discarded by the | ||
| post-script — the work is lost. | ||
|
|
||
| You wrote the initial output file in step 3. Confirm it still exists | ||
| and contains the correct target branch: | ||
|
|
||
| ```bash | ||
| echo "::notice::STEP 11: Validate structured output" | ||
| cat "${FULLSEND_OUTPUT_DIR}/agent-result.json" | ||
| ``` | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[low] edge-case
The 10c-verify recovery path says Go back to step 10a and stage and commit them when uncommitted changes are found, but does not limit how many times this loop can execute. If git commit keeps failing silently, the agent could loop between 10a and 10c-verify indefinitely.
Suggested fix: Consider adding a note such as If the commit still does not appear after one retry, report the failure in structured output and stop to match the bounded-retry pattern used elsewhere (step 9c MAX_RETRIES).