ci: add QA protocol enforcement checks#92
Conversation
…rue) The Python SDK used ensure_ascii=False in json.dumps during canonicalization, while the TypeScript SDK uses JSON.stringify which defaults to escaping non-ASCII characters. This caused cross-SDK hash disagreement for any protocol message containing non-ASCII Unicode (accents, CJK, emoji, etc.), breaking event hash chain integrity. JA-TM-01 in the Jarvis threat model.
Wires two reusable workflows from Flow-Research/.github: - credential-scanner: flags plaintext credentials in source files - pr-template-linter: validates PR body has required sections from template
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe validation workflow now handles edited pull requests and runs credential scanning and pull request linting. Protocol canonicalization now escapes non-ASCII characters, changing canonical strings and resulting hashes. ChangesWorkflow validation
Protocol canonicalization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: a8233319b9
ℹ️ 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".
|
|
||
| def canonicalize_protocol_value(value: Any) -> str: | ||
| return json.dumps(_sort_protocol_value(value), separators=(",", ":"), ensure_ascii=False) | ||
| return json.dumps(_sort_protocol_value(value), separators=(",", ":"), ensure_ascii=True) |
There was a problem hiding this comment.
For protocol records containing non-ASCII text, this now produces a different canonical string and therefore a different hash_protocol_value than the TypeScript helper: Python emits {"text":"\u00e9"} while JSON.stringify({text:'é'}) emits {"text":"é"}. Since event hashes and evidence hashes are derived from this serialization, mixed Python/TypeScript implementations will fail to verify otherwise valid Unicode records such as worker names, request notes, or review text; keep ensure_ascii=False or change both SDKs together.
Useful? React with 👍 / 👎.
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
The regenerate script uses os.path.join which produces Windows backslashes. CI runs on Linux and expects forward slashes. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/validate.yml:
- Line 5: Update the validate and credential-scan jobs in the workflow to run
only when github.event.action is not edited, while leaving the pr-lint job able
to handle edited pull request events. Preserve execution for push and other
non-PR events where the action is empty.
In `@packages/python/src/jarvis_protocol/__init__.py`:
- Line 1733: Align hash canonicalization between the Python serializer in
hash_protocol_value and the TypeScript serializer in
packages/typescript/src/index.js so identical payloads, including non-ASCII
strings, produce identical digests; either make both serializers use the same
Unicode escaping behavior or explicitly version the hash contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a6df7cf4-611d-4ef5-bb54-c4c2affe03da
📒 Files selected for processing (2)
.github/workflows/validate.ymlpackages/python/src/jarvis_protocol/__init__.py
|
|
||
| on: | ||
| pull_request: | ||
| types: [opened, synchronize, reopened, edited] |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Skip heavy and unrelated jobs on PR edits.
Including edited in the pull_request trigger means that every time the PR title or body is modified, all jobs in this workflow will rerun. Since only the new pr-lint job actually inspects the PR body, rerunning the extensive validate test suite and credential-scan on simple text edits wastes CI minutes and delays feedback.
Consider skipping the unrelated jobs when the event action is edited.
⚡ Proposed optimization for the jobs
Apply the following condition to the validate job:
validate:
+ if: github.event.action != 'edited'
runs-on: ubuntu-latest
steps:And apply the same condition to the credential-scan job:
credential-scan:
name: Credential Scanner
+ if: github.event.action != 'edited'
uses: Flow-Research/.github/.github/workflows/credential-scanner.yml@e519272096a29214dc6f4f36f1a7de2fc6fba96a(Note: For push events or other non-PR events, github.event.action is empty, so != 'edited' correctly evaluates to true and allows the jobs to run).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/validate.yml at line 5, Update the validate and
credential-scan jobs in the workflow to run only when github.event.action is not
edited, while leaving the pr-lint job able to handle edited pull request events.
Preserve execution for push and other non-PR events where the action is empty.
|
|
||
| def canonicalize_protocol_value(value: Any) -> str: | ||
| return json.dumps(_sort_protocol_value(value), separators=(",", ":"), ensure_ascii=False) | ||
| return json.dumps(_sort_protocol_value(value), separators=(",", ":"), ensure_ascii=True) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
echo "=== file outline ==="
ast-grep outline packages/python/src/jarvis_protocol/__init__.py --view expanded || true
echo "=== relevant symbols ==="
rg -n "def hash_protocol_value|ensure_ascii|_sort_protocol_value|hash_protocol_value\(" packages/python/src/jarvis_protocol/__init__.py packages/python -S
echo "=== tests mentioning hash_protocol_value or protocol hash ==="
rg -n "hash_protocol_value|protocol hash|ensure_ascii|json.dumps|_sort_protocol_value" packages/python -S
echo "=== file segment around hash_protocol_value ==="
sed -n '1700,1765p' packages/python/src/jarvis_protocol/__init__.pyRepository: Flow-Research/jarvis
Length of output: 11430
🏁 Script executed:
set -euo pipefail
echo "=== hash tests in python ==="
sed -n '730,785p' packages/python/tests/test_helpers.py
echo "=== README contract mentions ==="
sed -n '1,120p' packages/python/README.md
echo "=== cross-SDK / canonicalization search ==="
rg -n "canonicalize_protocol_value|hash_protocol_value|ensure_ascii|sha256|canonical" packages -SRepository: Flow-Research/jarvis
Length of output: 50378
🏁 Script executed:
set -euo pipefail
sed -n '730,785p' packages/python/tests/test_helpers.py
printf '\n=== README ===\n'
sed -n '1,120p' packages/python/README.md
printf '\n=== repository search ===\n'
rg -n "canonicalize_protocol_value|hash_protocol_value|ensure_ascii|sha256|canonical" packages -SRepository: Flow-Research/jarvis
Length of output: 50378
🏁 Script executed:
set -euo pipefail
echo "=== TypeScript canonicalizeProtocolValue ==="
sed -n '1370,1415p' packages/typescript/src/index.js
echo "=== Python/TS Unicode serialization probe ==="
python3 - <<'PY'
import json
value = {"text": "café", "emoji": "⚙️"}
print("python:", json.dumps(value, separators=(",", ":"), ensure_ascii=True))
print("python_no_ascii:", json.dumps(value, separators=(",", ":"), ensure_ascii=False))
PY
node - <<'JS'
const value = {text: "café", emoji: "⚙️"};
console.log("node:", JSON.stringify(value));
JSRepository: Flow-Research/jarvis
Length of output: 1693
Preserve the shared hash canonicalization. packages/python/src/jarvis_protocol/__init__.py:1732-1733 now escapes non-ASCII characters, but packages/typescript/src/index.js still serializes the same values as raw Unicode. That makes hash_protocol_value() produce different digests for the same payload across SDKs whenever strings contain non-ASCII data. Align the canonicalization or version the hash contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/python/src/jarvis_protocol/__init__.py` at line 1733, Align hash
canonicalization between the Python serializer in hash_protocol_value and the
TypeScript serializer in packages/typescript/src/index.js so identical payloads,
including non-ASCII strings, produce identical digests; either make both
serializers use the same Unicode escaping behavior or explicitly version the
hash contract.
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
1 participant
Scope
Add organization QA workflow enforcement to Jarvis CI and pin reusable workflows to immutable revisions.
Invariants
Design
Use the centralized Flow Research credential scanner and PR-template linter as reusable workflows.
Duplicate and idempotency review
CI checks are read-only and repeatable. No protocol records or data migrations are introduced.
Privacy and security review
Reusable workflow references are pinned to reviewed commit SHAs. Credential scanning remains enforced.
Verification
Summary by CodeRabbit
Bug Fixes
Chores