fix(devops): transport exact pre-push evidence - #493
Conversation
🤖 CodeAnt AI — Review Status
|
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reachedNext included review available in 11 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 115 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
Warning Your free Security trial is over. An organization admin can activate billing to continue. Comment |
Reviewer's GuideThe PR fixes pre-push evidence loss by parsing stdin once, handing a validated serialized update stream to both outgoing signature verification and local admission, and resolving changed paths fail-closed with NUL-safe Git discovery across supported ref-update scenarios. Sequence diagram for shared pre-push evidence transportsequenceDiagram
actor Git
participant PrePush as pre-push hook
participant Signing as verify-outgoing
participant Admission as ci-prepush-lowend
participant Core as signing-core
Git->>PrePush: send pre-push ref-update stdin
PrePush->>Core: parsePrePushInput(input)
Core-->>PrePush: RefUpdate[]
PrePush->>Core: serializePrePushUpdates(updates)
Core-->>PrePush: serialized updates
PrePush->>Signing: run with WORLD_SCRIPT_PREPUSH_UPDATES
Signing->>Core: parseSerializedPrePushUpdates(serialized)
Core-->>Signing: RefUpdate[]
Signing->>Core: verifyOutgoingUpdates(updates)
Signing-->>PrePush: signing result
PrePush->>Admission: run with WORLD_SCRIPT_PREPUSH_UPDATES
Admission->>Core: parseSerializedPrePushUpdates(serialized)
Core->>Core: resolvePushEvidence(updates)
Core-->>Admission: resolved or invalid evidence
Admission-->>Git: accept or fail closed
Flow diagram for fail-closed push evidence resolutionflowchart TD
A[Serialized pre-push updates] --> B[parseSerializedPrePushUpdates]
B --> C[resolvePushEvidence]
C --> D{Ref update valid?}
D -- No --> X[INVALID evidence and reject]
D -- Yes --> E{Deletion or tag?}
E -- Yes --> F[Record disposition]
E -- No --> G[Validate local and remote objects]
G --> H{Objects available?}
H -- No --> X
H -- Yes --> I[changedFilesBetween with NUL-safe Git paths]
I --> J{Git resolution succeeds?}
J -- No --> X
J -- Yes --> K[RESOLVED evidence for local admission]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
codeant-ai
Bot
added
the
size:L
🏁 CodeAnt Quality Gate ResultsCommit: ✅ Overall Status: PASSEDQuality Gate Details
|
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR successfully implements the pre-push evidence transport mechanism that addresses the caller/evidence boundary defect identified in PR #492. The implementation correctly ensures that pre-push stdin is consumed once and then shared between outgoing signature verification and local admission processes.
Key Strengths
Robust Architecture: The implementation establishes a clean separation of concerns with canonical parsing (parsePrePushInput), serialization (serializePrePushUpdates), and evidence resolution (resolvePushEvidence) functions. The evidence is captured once in the hook and then transported via environment variable to downstream consumers.
Comprehensive Error Handling: All functions implement fail-closed semantics with proper validation and error messages for malformed input, missing objects, and Git failures. The resolvePushEvidence function properly handles deletions, tags, new branches, and multiple updates.
Strong Test Coverage: The test suite validates all edge cases including deletions, new branches, malformed input, path resolution with special characters (newlines, Unicode, tabs), and round-trip serialization. Tests verify both local signing controls and GitHub API integration.
NUL-safe File Discovery: The implementation correctly uses -z flag in git diff to handle filenames with special characters and splits output on NUL bytes, ensuring lossless transport of changed paths.
Validation Results
All validation checks reported in the PR description have passed:
- ✅ 14/14 unit tests passed
- ✅ Biome checks passed on all files
- ✅
git diff --checkpassed - ✅
pnpm run signing:doctorpassed - ✅ SSH-signed commit verification working
- ✅ Pre-push checks reaching existing gates
The code is ready for merge.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
|
|
||
| export function resolvePushEvidence(input, cwd = process.cwd(), dependencies = {}) { | ||
| try { | ||
| const updates = Array.isArray(input) ? input : parsePrePushInput(input); |
There was a problem hiding this comment.
Type handling bug: resolvePushEvidence() doesn't properly handle string[] input despite accepting it in the type signature. When input is a string[], the code treats it as RefUpdate[] without parsing, which will cause runtime failures when accessing properties like update.localSha at lines 304-306.
The function should check if the array contains strings and parse accordingly:
const updates = Array.isArray(input)
? input.every((item) => typeof item === 'string')
? parsePrePushInput(input.join('\n'))
: input
: parsePrePushInput(input);This matches the pattern used in verifyOutgoingUpdates() at lines 442-445 and aligns with the TypeScript signature that declares input: string | string[] | RefUpdate[].
| const updates = Array.isArray(input) ? input : parsePrePushInput(input); | |
| const updates = Array.isArray(input) | |
| ? input.every((item) => typeof item === 'string') | |
| ? parsePrePushInput(input.join('\n')) | |
| : input | |
| : parsePrePushInput(input); | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
|
S3a exact-head status (
This is outside S3a scope, so no remediation commit or push is being made. S3a remains frozen pending the baseline doc-metrics owner/fix. |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| const base = isZeroSha(update.remoteSha) ? EMPTY_TREE : update.remoteSha; | ||
| if (!isZeroSha(base) && !commitExists(base)) |
There was a problem hiding this comment.
Suggestion: EMPTY_TREE is a tree object, but the default commitExists check requires ${sha}^{commit}. Consequently, every new-branch update uses EMPTY_TREE as its base and is rejected as an unavailable remote base before changed paths can be resolved. Skip commit validation for the empty-tree base or validate it as a tree object. [logic error]
Severity Level: Major ⚠️
- ❌ New-branch pushes fail during local pre-push admission.
- ⚠️ Changed-path evidence is never resolved for new branches.
- ⚠️ The failure affects the hook's normal first-push workflow.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** scripts/signing/signing-core.mjs
**Line:** 320:321
**Comment:**
*Logic Error: `EMPTY_TREE` is a tree object, but the default `commitExists` check requires `${sha}^{commit}`. Consequently, every new-branch update uses `EMPTY_TREE` as its base and is rejected as an unavailable remote base before changed paths can be resolved. Skip commit validation for the empty-tree base or validate it as a tree object.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| for await (const chunk of process.stdin) input += chunk; | ||
| try { | ||
| const updates = parsePrePushInput(input); | ||
| process.env.WORLD_SCRIPT_PREPUSH_UPDATES = serializePrePushUpdates(updates); |
There was a problem hiding this comment.
Suggestion: Storing the complete update stream in WORLD_SCRIPT_PREPUSH_UPDATES puts the serialized data into the child processes' inherited environment. Large multi-ref pushes can exceed the operating system's environment/argument-size limit, causing spawnSync in runNodeScript to fail and rejecting an otherwise valid push. Use a bounded file or pipe-based handoff instead of environment transport. [possible bug]
Severity Level: Major ⚠️
- ❌ Large multi-ref pushes are rejected locally.
- ⚠️ Failure occurs before signing and admission checks run.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** scripts/hooks/pre-push.mjs
**Line:** 10:10
**Comment:**
*Possible Bug: Storing the complete update stream in `WORLD_SCRIPT_PREPUSH_UPDATES` puts the serialized data into the child processes' inherited environment. Large multi-ref pushes can exceed the operating system's environment/argument-size limit, causing `spawnSync` in `runNodeScript` to fail and rejecting an otherwise valid push. Use a bounded file or pipe-based handoff instead of environment transport.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix![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: 7517c4a2a1
ℹ️ 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".
| const lines = input.split(/\r?\n/).filter((line) => line.length > 0); | ||
| if (lines.length === 0) throw new Error('pre-push input is empty'); |
There was a problem hiding this comment.
When git push finds every requested ref already up to date, Git still invokes the pre-push hook but supplies an empty stdin stream (confirmed with Git 2.43 against a local bare remote). This new check therefore throws pre-push input is empty, turns an otherwise successful no-op push into a failure, and exits before the repository's mandatory quick pre-push gate runs; treat zero updates as a valid stream with no objects to verify.
AGENTS.md reference: AGENTS.md:L297-L303
Useful? React with 👍 / 👎.
|
|
||
| export function resolvePushEvidence(input, cwd = process.cwd(), dependencies = {}) { | ||
| try { | ||
| const updates = Array.isArray(input) ? input : parsePrePushInput(input); |
There was a problem hiding this comment.
The new declaration explicitly permits resolvePushEvidence(string[]), but this branch treats every array as already-parsed RefUpdate objects. A typed caller passing valid pre-push lines consequently reads localSha and remoteSha from strings and returns evidenceState: 'INVALID'; distinguish string arrays and pass them through parsePrePushInput, as verifyOutgoingUpdates already does.
Useful? React with 👍 / 👎.
| if (!isZeroSha(base) && !commitExists(base)) | ||
| throw new Error(`remote base object is unavailable for ${update.remoteRef}`); |
There was a problem hiding this comment.
When creating a remote feature branch, Git supplies a zero remote SHA, so the preceding line substitutes EMPTY_TREE; this condition then calls the default commitExists, which runs git cat-file -e <empty-tree>^{commit} and fails because that object is a tree rather than a commit. Consequently resolvePushEvidence returns INVALID and the hook rejects every first push of a branch; skip commit-only validation for the synthetic base or validate it as a tree before diffing. This blocks the repository-required feature-branch PR workflow.
AGENTS.md reference: AGENTS.md:L407-L407
Useful? React with 👍 / 👎.
| updates = | ||
| Array.isArray(input) && input.every((item) => typeof item === 'string') | ||
| ? parsePrePushInput(input.join('\n')) | ||
| : input; |
There was a problem hiding this comment.
The updated declaration permits verifyOutgoingUpdates(string), but this branch parses only arrays whose elements are strings and otherwise assigns the raw value directly to updates. A typed caller passing one canonical pre-push stream is therefore rejected by the following Array.isArray check even though wrapping the same line in an array succeeds; route raw strings through parsePrePushInput as well.
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
1 participant
User description
Purpose
This replacement slice reconstructs the outgoing pre-push range and lossless
change-evidence transport from the frozen PR #491 source:
9bbeded78f1032a5e74aa370ef7ca158628ad784PR #492 exposed the caller/evidence boundary defect: pre-push stdin was consumed
by outgoing signature verification before local admission could receive the same
ref-update evidence.
Scope
This PR owns only:
multiple-update, malformed-input, and Git-failure cases;
The S1 classifier corrections remain deferred until this transport contract is
available. S3b exact-tree dependency compatibility proof, node_modules
mirroring, exact-tree TypeScript execution, and broad localSha execution
deduplication remain separate slices.
Validation
tests/unit/signing.test.ts: 14/14 passed;git diff --check: passed;pnpm run signing:doctor: passed;pnpm run ci:prepushreached the existing release/doc truth gate aftertypecheck and i18n passed, then reported the pre-existing README metric drift
(6954 vs expected 6958 tests). No documentation change is included here.
Non-goals
main;changes.
Summary by Sourcery
Preserve exact pre-push ref and path evidence across signing and local admission checks, failing closed when it cannot be resolved.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
CodeAnt-AI Description
Preserve and validate exact pre-push evidence across signing and local checks
What Changed
Impact
✅ Consistent signing and local admission results✅ Fewer pushes accepted with incomplete evidence✅ Reliable handling of unusual filenames💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.