feat(signing): add working-tree-vs-push divergence detection (S3b Part 1) - #501
Merged
Conversation
…t 1) Neither pathEvidenceState (#498) nor any other current signal proves that the content ci-prepush-lowend.mjs's local checks actually validate (everything runs against whatever is on disk, cwd: projectRoot) corresponds to localSha's exact committed tree, rather than a working tree that has drifted from it (uncommitted edits, staged-but-uncommitted changes, wrong ref checked out). Required CI remains the sole merge-safety authority regardless, so this is a local-gate trust/DX gap, not a safety hole -- and this commit closes only that gap, as a diagnostic signal, not as exact-tree validation. resolvePushEvidence() (signing-core.mjs) gains a new orthogonal dimension, workingTreeState: 'MATCHES' | 'DIVERGED' | 'NOT_APPLICABLE' | 'UNKNOWN', computed per-update via a new computeWorkingTreeState() using `git diff --quiet <localSha> --` (compares working tree directly against the commit's tree; structurally cannot see untracked files, which is exactly why MATCHES is not sufficient to prove exact-tree equivalence -- an untracked file satisfying an import that's absent from the real commit would make a local check pass when a real checkout of localSha would fail). DELETED updates get NOT_APPLICABLE (no commit to compare against); TAG updates are included on the same terms as branch updates via the same call (git's own revision parsing transitively peels a tag to its target), not excluded as an earlier draft had them -- a diverged tag push is now reported instead of silently unreported. Aggregation precedence: DIVERGED > UNKNOWN > MATCHES > (NOT_APPLICABLE only when no relevant update exists, e.g. an all-deletions push). computeWorkingTreeState() never throws -- it maps any runGit() failure (spawn error, timeout) to UNKNOWN internally, and critically checks result.error before trusting result.status, since runGit's own `status: result.status ?? 1` fallback makes a killed/failed subprocess indistinguishable from a genuine "differences found" exit code 1 unless the caller inspects error first. This keeps the new diagnostic fully isolated from resolvePushEvidence's existing canonical try/catch: a diagnostic failure can only ever produce workingTreeState: 'UNKNOWN' on the RESOLVED path, never evidenceState: 'INVALID'. This matters because ci-prepush-range-resolver.mjs's resolveManualEvidence() throws whenever evidenceState !== 'RESOLVED' -- without this isolation, a transient git timeout on the new diagnostic would have turned otherwise-valid, already-verified #494/#498 evidence into a hard pre-push rejection. Propagated through resolveManualEvidence()'s evidence-file path unchanged; the manual/no-evidence-file path (changedFilesFromManualRange, direct `pnpm run ci:prepush` invocation) reports NOT_APPLICABLE, not MATCHES -- there is no localSha and no push event in that mode, so no comparison of any kind happens, and claiming MATCHES would assert an equivalence that was never checked. ci-prepush-lowend.mjs reports a non-blocking, informational line only on DIVERGED or UNKNOWN (MATCHES/NOT_APPLICABLE stay silent) -- deliberately not coupled to the existing `full`-admission escalation lever, since divergence and incomplete-change-evidence are different failure modes with no shared remedy. Deliberately not implemented in this slice (reserved for Part 2, an isolated-localSha-worktree exact verification mechanism to be designed fresh against then-current main): MATCHES is never treated as a skip condition for exact verification anywhere in this codebase, in this commit or otherwise -- proving tracked-content equivalence sufficient to safely skip real verification would require also ruling out untracked/gitignored drift, which is exactly what an isolated worktree proves and a cheap diff cannot. Tests: computeWorkingTreeState's runGit-error-vs-status-1 distinction (the exact ambiguity this commit exists to close) tested directly via injected runGit results; the UNKNOWN-does-not-mutate-evidenceState regression proven directly, not just asserted in prose; tag inclusion, deletion/manual NOT_APPLICABLE, and aggregation precedence (including a mixed DIVERGED+UNKNOWN and MATCHES+UNKNOWN case) all covered deterministically with no real git subprocess in the new tests. Four pre-existing resolvePushEvidence tests updated to inject worktreeMatchesCommit so they stay git-subprocess-free rather than incidentally exercising the real default against fake SHAs. README test-count badges resynced via `pnpm run sync:readme` (578 files / 6997+ tests). Validated: full `pnpm run ci:prepush`, `pnpm run lint`, `pnpm run typecheck` (4-checker), and both targeted test files all pass.
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
🤖 CodeAnt AI — Review Status
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Reviewer's GuideIntroduces a diagnostic-only working-tree-vs-pushed-commit divergence signal, computed per branch and tag update and propagated to local pre-push reporting, while deliberately leaving canonical evidence validity, admission behavior, and future exact-tree verification unchanged; comprehensive DI-based tests cover failure handling and state semantics. Sequence diagram for working-tree divergence diagnosticssequenceDiagram
participant Resolver as resolvePushEvidence
participant Git as git diff --quiet
participant Evidence as PushEvidence
participant Prepush as ci-prepush-lowend.mjs
Resolver->>Git: computeWorkingTreeState(localSha)
alt result.error
Git-->>Resolver: error
Resolver->>Evidence: workingTreeState = UNKNOWN
else status = 0
Git-->>Resolver: status 0
Resolver->>Evidence: workingTreeState = MATCHES
else status = 1
Git-->>Resolver: status 1
Resolver->>Evidence: workingTreeState = DIVERGED
else other status
Git-->>Resolver: status
Resolver->>Evidence: workingTreeState = UNKNOWN
end
Resolver->>Evidence: aggregateWorkingTreeState(updates)
Evidence-->>Prepush: diagnostic state
alt DIVERGED or UNKNOWN
Prepush->>Prepush: report informational line
end
Flow diagram for working-tree state aggregationflowchart TD
A[Push or manual evidence resolution] --> B{Push update has localSha?}
B -->|No, deletion| C[NOT_APPLICABLE]
B -->|Yes| D["computeWorkingTreeState(localSha)"]
D --> E{git diff --quiet result}
E -->|error or other status| F[UNKNOWN]
E -->|status 0| G[MATCHES]
E -->|status 1| H[DIVERGED]
C --> I[aggregateWorkingTreeState]
F --> I
G --> I
H --> I
I --> J{Precedence}
J -->|Any DIVERGED| K[DIVERGED]
J -->|Else any UNKNOWN| L[UNKNOWN]
J -->|Else all MATCHES| M[MATCHES]
J -->|No comparable updates| N[NOT_APPLICABLE]
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.
This PR implements working-tree divergence detection as a diagnostic signal with careful isolation from canonical evidence validity. The implementation correctly handles error vs. exit-code distinction, aggregates states with proper precedence, and includes comprehensive test coverage proving the diagnostic cannot corrupt evidence validity. No blocking defects identified - the code functions correctly as designed.
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.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="scripts/signing/signing-core.mjs" line_range="336" />
<code_context>
}
+// QNBS-v3: diagnostic-only signal; never throws, so it cannot corrupt canonical evidence validity.
+export function computeWorkingTreeState(sha, cwd, dependencies = {}) {
+ const runGitFn = dependencies.runGit ?? runGit;
+ const result = runGitFn(['diff', '--quiet', sha, '--'], { cwd });
+ if (result.error) return 'UNKNOWN';
+ if (result.status === 0) return 'MATCHES';
+ if (result.status === 1) return 'DIVERGED';
+ return 'UNKNOWN';
+}
+
</code_context>
<issue_to_address>
**issue (bug_risk):** `computeWorkingTreeState()` does not catch exceptions from `runGitFn()`, so a thrown subprocess error escapes into `resolvePushEvidence()`'s canonical catch and converts otherwise-valid evidence into `evidenceState: 'INVALID'` instead of the promised diagnostic-only `workingTreeState: 'UNKNOWN'`.
**Triggers:** When the injected or underlying git runner throws rather than returning a result with `error`.
**Suggested fix:** Wrap the `runGitFn(...)` call in `try/catch` and return `'UNKNOWN'` from the catch block.
```suggestion
let result;
try {
result = runGitFn(['diff', '--quiet', sha, '--'], { cwd });
} catch {
return 'UNKNOWN';
}
```
</issue_to_address>Sourcery assessment
Approval pending. 1 finding to address first.
Blocking findings: scripts/signing/signing-core.mjs:336
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Warning Review limit reachedNext included review available in 42 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 111 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 (3)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. 📝 WalkthroughWalkthroughThe change adds working-tree state diagnostics to push evidence, propagates the state through pre-push range resolution, reports divergent or unknown states, expands test coverage, and updates README test metrics. ChangesWorking-tree diagnostics
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🟡 Moderate · up to A duplicate declaration prevents the signing test file from parsing and blocks the affected test suite from running; it should be removed before merging. Sequence Diagram(s)sequenceDiagram
participant PushEvidence
participant WorkingTreeDiagnostic
participant Git
participant PrepushReporter
PushEvidence->>WorkingTreeDiagnostic: classify update state
WorkingTreeDiagnostic->>Git: run git diff --quiet
Git-->>WorkingTreeDiagnostic: comparison result or error
WorkingTreeDiagnostic-->>PushEvidence: MATCHES, DIVERGED, or UNKNOWN
PushEvidence->>PrepushReporter: provide workingTreeState
PrepushReporter-->>PrepushReporter: report diagnostic state
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 7 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
…orkingTreeState
The default runGit() never throws (spawnSync failures are captured
into result.error, not thrown), so this was unreachable through the
real production call path. But computeWorkingTreeState() is an
exported function with an optional dependencies.runGit override, and
its whole contract -- documented in its own QNBS-v3 comment and this
slice's commit message -- is that it never throws, so a diagnostic
failure can never corrupt canonical evidence validity via
resolvePushEvidence's outer try/catch. A future caller (or test)
providing a runGit-shaped dependency that throws instead of returning
{error} would have silently broken that guarantee. Wrap the call in
its own try/catch and return UNKNOWN, matching the existing handling
for a returned result.error.
README test-count badges resynced via `pnpm run sync:readme` for the
added regression test (578 files / 6998+ tests).
Validated: full `pnpm run ci:prepush`, `pnpm run lint`, `pnpm run
typecheck` (4-checker), and both targeted test files all pass.
Owner
Author
|
@coderabbitai 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: d061c8e9ac
ℹ️ 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".
…ope explicit inline chatgpt-codex-connector correctly observed that an untracked file can make git diff --quiet <sha> -- report MATCHES even when that file is absent from the pushed commit (e.g. an untracked ambient .d.ts satisfying an import the real commit is missing). This is not a new gap -- it is the exact, deliberately-scoped limitation this slice's design already reasons through: MATCHES is documented (commit message, PR description, the report() wording in ci-prepush-lowend.mjs) as "no tracked-content divergence detected", never "verified"/"validated", and is never used anywhere as a skip condition for real verification. Widening this check to also flag any untracked file (e.g. via `git ls-files --others --exclude-standard`) was evaluated and rejected: it would reintroduce exactly the false-positive-on-an-unrelated-scratch- file problem this design avoids by construction today (a benign WIP file anywhere in the repo would mark every push DIVERGED, making the signal noisy enough to lose developer trust), and closing the gap soundly would additionally require ruling out gitignored/generated- artifact drift -- at which point the "cheap detector" has grown into the isolated worktree that is Part 2's job, not Part 1's. Added a one-line inline comment at the exact call site making this scope boundary explicit in the code itself, not only in the commit message and PR description, since a reviewer found it non-obvious enough to flag independently. No behavior change. Validated: full `pnpm run ci:prepush`, `pnpm run lint`, `pnpm run typecheck` (4-checker), and both targeted test files all pass.
Owner
Author
|
@coderabbitai review |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
7 tasks
qnbs
added a commit
that referenced
this pull request
Aug 25, 2026
…roof (S3b Part 2a) (#502) Part 2a of the S3b reconstruction slice (follows #501). Adds a diagnostic-only dependencyState dimension mirroring workingTreeState's 4-state model, comparing pushed commits' dependency manifests against the local reconciled baseline entirely via git objects, no worktree. Full remediation history: - aca9dbb: initial implementation - 3abe3ba: fixed byte-identity (UTF-8 decoding vs raw bytes), CRLF/autocrlf false-DIVERGED, and diagnostic-isolation throw-safety (Sourcery + CodeRabbit + chatgpt-codex-connector findings) - 4c019c3: fixed git ls-tree full-tree scoping and default path C-quoting (chatgpt-codex-connector finding) Review epoch reached quiescence: 0 unresolved threads across all 3 channels (reviewThreads, issue comments, review bodies), fresh Sourcery APPROVED + Amazon Q "ready for merge" on the exact final commit, all 29 applicable CI checks green.
This was referenced Aug 25, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
User description
Summary
S3b Part 1 of the PR #491 program-continuity reconstruction — the smallest prerequisite slice for isolated exact-
localShaverification (Part 2, a separate, dedicated future PR).Neither
pathEvidenceState(#498) nor any other current signal proves that the contentci-prepush-lowend.mjs's local checks actually validate (everything runs against whatever is on disk) corresponds tolocalSha's exact committed tree, rather than a working tree that has drifted from it. Required CI remains the sole merge-safety authority regardless — this is a local-gate trust/DX gap, not a safety hole, and this PR closes only that gap as a diagnostic signal, not as exact-tree validation.resolvePushEvidence()gainsworkingTreeState: 'MATCHES' | 'DIVERGED' | 'NOT_APPLICABLE' | 'UNKNOWN', computed per-update viagit diff --quiet <localSha> --.MATCHESis not exact-tree proof (it can't see untracked files) and is never used anywhere as a skip condition for future exact verification.UNKNOWNnever rewritesevidenceState/pathEvidenceState. This matters concretely:resolveManualEvidence()throws wheneverevidenceState !== 'RESOLVED', so without this isolation a transient git timeout on the new diagnostic would have turned otherwise-valid, already-verified fix(devops): replace pre-push evidence handoff #494/fix(devops): make pre-push path evidence completeness explicit #498 evidence into a hard pre-push rejection.computeWorkingTreeState()checksresult.errorbefore trustingresult.status, sincerunGit's ownstatus: result.status ?? 1fallback makes a killed/failed subprocess indistinguishable from a genuine "differences found" exit code 1 otherwise.NOT_APPLICABLE, notMATCHES— nolocalShaexists to compare against in that mode.ci-prepush-lowend.mjsreports one non-blocking, informational line onDIVERGED/UNKNOWNonly, not coupled to the existingfull-admission escalation lever.Reserved for Part 2 (isolated-
localSha-worktree exact verification, designed fresh against then-currentmain):MATCHESis never a skip gate anywhere in this codebase.Test plan
computeWorkingTreeState's error-vs-status-1 distinction tested directly; the UNKNOWN-does-not-mutate-evidenceStateregression proven directly; tag inclusion, deletion/manualNOT_APPLICABLE, and aggregation precedence (DIVERGED>UNKNOWN>MATCHES) all covered.resolvePushEvidencetests updated to injectworktreeMatchesCommitso they stay git-subprocess-free.pnpm exec vitest run tests/unit/signing.test.ts tests/unit/tooling/ciPrepushRangeResolver.test.ts— 41/41 pass.pnpm run ci:prepush— full local admission gate passes end-to-end.pnpm run lint— clean (0 warnings; 6 pre-existing infos in an unrelated file).pnpm run typecheck(4-checker, CI-equivalent) — clean.Summary by Sourcery
Detect working-tree divergence from pushed commits while preserving existing evidence validation and required-CI merge safety.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests:
CodeAnt-AI Description
Detect when local checks do not match the commits being pushed
What Changed
DIVERGEDorUNKNOWNresults while keeping required CI and existing evidence validation unchanged.Impact
✅ Clearer local push verification✅ Earlier detection of uncommitted changes affecting push checks✅ Fewer false pre-push rejections after Git command failures💡 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.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation