Repository navigation
feat(security): supply-chain security - SBOM, dependency pinning and provenance verification - #1001
Conversation
…venance verification - Pin all third-party GitHub Actions to immutable commit SHAs - Add SBOM generation job (CycloneDX) for API, frontend, intelligence, contracts - Add typosquat detection for new npm dependencies - Update SECURITY.md with supply-chain security policy documentation - Update .vulnignore with time-bounded review dates and ownership Closes Suncrest-Labs#867
❌ Deploy Preview for nesterhq failed. Why did it fail? →
|
❌ Deploy Preview for nesterdapp failed. Why did it fail? →
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughCI workflows now pin actions, validate vulnerability waivers, detect npm typosquats, and generate SBOMs. API migrations add persistence structures. Intelligence, risk, and contract code receive typing, classification, failure-tracking, and compatibility updates. ChangesSupply-chain security
API schema migrations
Service and contract compatibility
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation Code changes implement all key requirements from Full details: Out of Scope Changes checkExplanation All changes align with ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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/ci.yml:
- Around line 487-488: Update the SBOM generation and upload steps in the CI
workflow to fail when required generation or upload is incomplete: remove `||
true` suppression from the intentional fallback paths, preserve only the
intended fallback behavior, and change each relevant artifact upload’s
`if-no-files-found` setting from `warn` to `error`.
- Around line 307-324: The dependency scan should preserve full npm package
names, including scopes, instead of reducing names with dep.split('/')[-1].
Update the manifest comparison logic to load both base and head manifests,
inspect only dependencies newly added in the head, and compare their full
normalized names against known_packages while retaining the existing
suspicious-distance rules.
- Around line 476-495: Update the SBOM tool installation and invocation steps
around cyclonedx-gomod, syft, and cyclonedx-npm to use exact immutable release
versions instead of `@v1`, the mutable main branch installer, or an unversioned
npx package. Pin downloaded artifacts to their release checksums and verify
those checksums before execution, while preserving the existing SBOM generation
commands and fallback behavior.
- Around line 274-332: Fix the indentation of the embedded Python script under
the “Typosquat detection (new npm dependencies)” step’s `run: |` block so every
script line is valid YAML block content. Preserve the existing script logic and
ensure the closing Python code is correctly included within the shell command.
In @.github/workflows/load-soak.yml:
- Line 19: Update the actions/checkout step in the load-soak workflow to set
persist-credentials to false, ensuring the checked-out workspace mounted by the
k6 container does not contain the default GitHub token while preserving the
existing checkout behavior.
In @.vulnignore:
- Around line 11-14: Replace the hardcoded vulnerability-ID allowlist with
enforced parsing and validation of waiver metadata from .vulnignore, including
review date expiration, owner presence, and status. Update SECURITY.md lines
197-197 to describe .vulnignore as the enforcement source of truth, and update
lines 223-228 to state that expired, malformed, or ownerless waivers fail CI;
apply the implementation change at .vulnignore lines 11-14.
In `@SECURITY.md`:
- Around line 153-159: Update the CI workflow’s SBOM generation and upload steps
so SBOM creation is mandatory for every listed artifact: remove conditional
execution and failure suppression, and configure artifact uploads to fail when
expected SBOM files are missing instead of warning. Keep the SECURITY.md policy
aligned with the enforced behavior, or explicitly narrow both the documentation
and workflow to only changed artifacts.
- Around line 201-206: Update the Python security-check step in the workflow,
particularly the pip-audit or bandit handling described around the finding-count
output, so it exits with a nonzero status whenever high or critical findings are
detected. Preserve the existing reporting output and successful zero-finding
behavior, ensuring the documented production-dependency gate blocks
merges/releases as intended.
- Around line 163-169: Narrow the integrity statement in SECURITY.md to package
dependencies only, or update the SBOM toolchain references to document exact
version pins and integrity verification. Ensure the wording no longer claims all
dependencies are pinned while CI still uses unpinned cyclonedx-gomod, syft, and
cyclonedx-npm.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 13c9221d-7929-409c-bf79-794bea88673e
📒 Files selected for processing (7)
.github/workflows/ci.yml.github/workflows/contract-audit.yml.github/workflows/dependabot-auto-merge.yml.github/workflows/load-soak.yml.github/workflows/security.yml.vulnignoreSECURITY.md
The stable branch moved from 2c7215f1 to 4cda84d5, causing Setup Rust step to fail. Pinning to the new stable commit.
| run: | | ||
| # Check for typosquatting patterns in package.json dependencies | ||
| # Uses npm package name similarity to detect potential typosquats | ||
| python3 -c " |
There was a problem hiding this comment.
This block breaks the entire workflow file, not just this step.
run: | on line 276 opens a block scalar, but the Python starting here sits at column 0 — outside the scalar's indentation. YAML therefore ends the scalar at line 279 and tries to parse import json, sys, re as a new top-level mapping key:
yaml.scanner.ScannerError: while scanning a simple key
in "ci.yml", line 280, column 1
could not find expected ':'
GitHub Actions rejects a workflow it cannot parse, so merging this disables every job in ci.yml repo-wide — website, intelligence, dapp, api, contracts, security, all of it.
The 12 green checks on this PR are misleading: they ran against the merge-base ci.yml from dev, not this version. The step is also gated on needs.changes.outputs.dapp, which was false for this PR's own file set, so it never executed even locally.
Two ways to fix it. Either indent the whole heredoc to sit inside the block scalar, or — better, and what #968 already does — move the script to scripts/typosquat_check.py and invoke it as a file. A committed script is testable and lintable; an inlined one is neither.
Please validate before re-requesting review:
python -c "import yaml; yaml.safe_load(open('.github/workflows/ci.yml'))"
There was a problem hiding this comment.
@0xDeon thanks for the thorough review — both points are addressed. The YAML block-scalar indentation is fixed (the Python now sits inside the run: | block, and yaml.safe_load passes), and the missing ) SyntaxError is fixed. Per your suggestion, the logic was also extracted into a committed, testable scripts/typosquat_check.py (invoked as a file, with regression tests in scripts/test_typosquat_check.py), and the step now has continue-on-error: true so the fuzzy heuristic can never block an unrelated PR. Full scoped names are compared and only newly-added deps (incl. optionalDependencies) are checked.
| if d <= 2 and d > 0: | ||
| suspicious.append((dep, known, d)) | ||
|
|
||
| if suspicious: |
There was a problem hiding this comment.
Separate from the YAML problem above, this Python does not compile.
The print(...) call is missing its closing paren immediately before else::
print(f' {dep} (edit distance {dist} from {known})'
else:python3 -c compiles the whole string before executing, so this is a hard SyntaxError, not a runtime issue on an unusual path.
This step has no continue-on-error, so once the YAML is valid this would fail Security Scanning on every PR that touches dapp or shared. For an advisory heuristic like typosquat detection I'd suggest adding continue-on-error: true as well — a fuzzy name-similarity check shouldn't be able to block an unrelated PR from merging.
Worth noting the description also lists "Typosquat Detection" via scripts/typosquat_check.py, but that file isn't in this diff — it only exists in #968. Please align the description with what the branch actually contains.
There was a problem hiding this comment.
@0xDeon fixed: the script now compiles (closing paren added) and has been extracted into scripts/typosquat_check.py with continue-on-error: true on the step, per your suggestion. Regression tests are added at scripts/test_typosquat_check.py.
- govulncheck: parse and validate .vulnignore waiver metadata (review date expiry, owner, status); undocumented waivers fail the job - typosquat: diff base/head manifests, check only newly added deps with full scoped package names - sbom: pin cyclonedx-gomod@v1.10.0 and syft v1.50.0 with checksum verification; drop unpinned npx fallback; fail uploads when SBOMs are missing - load-soak: checkout with persist-credentials: false - security: pip-audit fails on high/critical findings; SECURITY.md documents enforced waiver/SBOM/typosquat policies
The embedded Python script inside the run block was unindented, making the entire workflow unparseable and silently disabling the x402 audit job. Indent the script to the block scalar so the workflow loads and the job can run again.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/security.yml (1)
258-270: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSecurity Misconfiguration (CWE-693)
Reachability: External
Fail on every
pip-auditfinding until severity data is available
.github/workflows/security.yml:258-270filters onseverityandcvss, butpip-auditJSON does not provide these fields. Findings are therefore omitted, and the workflow can pass despite vulnerabilities. Fail when any finding exists, or enrich the report with a severity-aware data source.🤖 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/security.yml around lines 258 - 270, Update the vulnerability collection loop in the security workflow to append every finding returned by pip-audit, without filtering on the unavailable severity or cvss fields. Keep the existing failure behavior in the `if vulns` block so any reported finding causes the workflow to exit with status 1.
🤖 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/ci.yml:
- Around line 365-370: Update the dep_names function to include keys from
optionalDependencies alongside dependencies and devDependencies, so newly added
optional packages are included in the existing typosquat detection and warning
flow.
- Around line 214-240: Replace the fixed 800-character metadata lookup in the
vulnerability validation blocks at .github/workflows/ci.yml lines 214-240 and
.github/workflows/security.yml lines 114-140 with explicit per-vulnerability
waiver block parsing, ensuring metadata cannot be inherited from another entry.
Require Justification, Review date, Owner, and Status within each block, and add
regression tests covering an ownerless appended vulnerability ID and an entry
missing Justification.
In @.vulnignore:
- Around line 20-31: Remove the stale GO-2026-5004 waiver block from
.vulnignore, including its justification, review metadata, owner, and status
comments; retain all other vulnerability entries unchanged.
In `@SECURITY.md`:
- Around line 163-169: Align the Python dependency integrity claim with the
implementation: update apps/intelligence/requirements.txt to use exact versions
with valid hashes, pin the pip-audit version, and configure the security
workflow’s installation and audit steps to enforce hashes with --require-hashes.
If this cannot be implemented, revise the SECURITY.md statement to accurately
describe the current Python dependency controls.
---
Outside diff comments:
In @.github/workflows/security.yml:
- Around line 258-270: Update the vulnerability collection loop in the security
workflow to append every finding returned by pip-audit, without filtering on the
unavailable severity or cvss fields. Keep the existing failure behavior in the
`if vulns` block so any reported finding causes the workflow to exit with status
1.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 415e8ff8-cf50-4dff-8432-bce57d7b5678
📒 Files selected for processing (5)
.github/workflows/ci.yml.github/workflows/load-soak.yml.github/workflows/security.yml.vulnignoreSECURITY.md
| def dep_names(pkg): | ||
| return set(pkg.get("dependencies", {})) | set(pkg.get("devDependencies", {})) | ||
|
|
||
| base_pkg = load_manifest(os.environ.get("BASE_REF", "")) | ||
| head_pkg = json.loads(Path(MANIFEST).read_text()) | ||
| new_deps = dep_names(head_pkg) - dep_names(base_pkg) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- workflow context ---'
sed -n '320,395p' .github/workflows/ci.yml
printf '%s\n' '--- related dependency-analysis references ---'
rg -n -C 3 'dep_names|new_deps|optionalDependencies|Levenshtein|levenshtein' .github/workflows/ci.ymlRepository: Suncrest-Labs/nester
Length of output: 4810
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
import json
from pathlib import Path
text = Path(".github/workflows/ci.yml").read_text()
start = text.index(" def dep_names(pkg):")
end = text.index("\n", text.index(" new_deps =", start))
source = text[start:end]
print("--- extracted logic ---")
print(source)
# Model the exact set operation used by the workflow without executing repository code.
def current_dep_names(pkg):
return set(pkg.get("dependencies", {})) | set(pkg.get("devDependencies", {}))
base = {"dependencies": {"safe-package": "^1.0.0"}}
head = {
"dependencies": {"safe-package": "^1.0.0"},
"optionalDependencies": {"typo-squatted-package": "^1.0.0"},
}
print("--- behavior probe ---")
print("current head names:", sorted(current_dep_names(head)))
print("current new names:", sorted(current_dep_names(head) - current_dep_names(base)))
print("optional package detected:", "typo-squatted-package" in
(current_dep_names(head) - current_dep_names(base)))
PYRepository: Suncrest-Labs/nester
Length of output: 610
Security Misconfiguration (CWE-693)
Reachability: External
Include optionalDependencies in dep_names at .github/workflows/ci.yml:365-366. New optional packages currently bypass the Levenshtein typosquat check and warning path.
🤖 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/ci.yml around lines 365 - 370, Update the dep_names
function to include keys from optionalDependencies alongside dependencies and
devDependencies, so newly added optional packages are included in the existing
typosquat detection and warning flow.
| All package dependencies are pinned to exact versions with integrity verification: | ||
| - **Go**: `go.sum` with checksum database (`GOSUMDB`) verification | ||
| - **npm**: Lockfile (`pnpm-lock.yaml`) with integrity hashes; CI uses `--frozen-lockfile` to reject unrecorded changes | ||
| - **Rust**: `Cargo.lock` with verified crate checksums | ||
| - **Python**: `requirements.txt` version pins audited by `pip-audit` in CI | ||
|
|
||
| CI fails the build if integrity verification fails for any ecosystem. CI tooling is pinned as well: third-party GitHub Actions are pinned to immutable commit SHAs (see below) and the SBOM generators (`cyclonedx-gomod`, `syft`) are installed from exact release versions, with downloaded binaries verified against published checksums. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- workflow references ---'
rg -n -C 8 'pip-audit|require-hashes|requirements\.txt|pip install|pip3? ' .github/workflows/security.yml 2>/dev/null || true
printf '%s\n' '--- Python dependency files ---'
git ls-files | rg '(^|/)(requirements[^/]*\.txt|pyproject\.toml|Pipfile(\.lock)?|.*requirements.*\.txt)$' || true
printf '%s\n' '--- relevant dependency contents ---'
while IFS= read -r f; do
printf '\n--- %s ---\n' "$f"
cat -n "$f"
done < <(git ls-files | rg '(^|/)(requirements[^/]*\.txt|pyproject\.toml|Pipfile(\.lock)?|.*requirements.*\.txt)$')Repository: Suncrest-Labs/nester
Length of output: 2215
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- workflow section ---'
sed -n '185,275p' .github/workflows/security.yml
printf '%s\n' '--- tracked Python dependency files ---'
for f in apps/intelligence/requirements.txt apps/intelligence/pyproject.toml; do
if [ -f "$f" ]; then
printf '\n--- %s ---\n' "$f"
cat -n "$f"
fi
done
printf '%s\n' '--- repository references to Python requirements ---'
rg -n -C 4 'apps/intelligence/requirements\.txt|(^|[^[:alnum:]_])requirements\.txt|pip-audit|require-hashes' \
--glob '!SECURITY.md' --glob '!.git/**' . || trueRepository: Suncrest-Labs/nester
Length of output: 8694
🌐 Web query:
pip-audit documentation --require-hashes requirements file hash enforcement
💡 Result:
In pip-audit, the --require-hashes flag is used to ensure that every dependency listed in a requirements file has an associated hash for repeatable, secure audits [1][2]. When this flag is enabled, pip-audit will fail if any dependency in the requirements file is missing a hash [1][2]. Key details regarding this feature include: Usage: It is designed to be used in conjunction with the -r or --requirement flag [3]. Behavior: The flag forces hash checking for all requirements [1][2]. If any package in a requirements file already includes a --hash option, --require-hashes is implicitly enabled [1][2]. It provides a mechanism similar to pip's own hash-checking mode, offering protection against potential tampering by ensuring all packages are verified against their expected hashes [4][2]. Considerations: When using the OSV service for vulnerability data, the --require-hashes flag primarily enforces the presence of hashes rather than their validity; using the PyPI service is recommended if you need to enforce hash validity [3]. It is considered a best practice for repeatable builds and audits, often used in deployment scripts to verify that dependencies are fully pinned and hashed [4][2]. For users who want to skip dependency resolution for already pinned and hashed files, using --require-hashes tells pip-audit to perform the audit on the existing file content without attempting to resolve the full dependency tree, which increases audit speed and reliability [1][2].
Citations:
- 1: https://github.com/pypa/pip-audit?tab=readme-ov-file
- 2: https://github.com/pypa/pip-audit/
- 3: https://github.com/pypa/pip-audit/blob/fe414d39/pip_audit/_cli.py
- 4: https://pip.pypa.io/en/stable/topics/secure-installs/
Python Dependency Integrity Is Not Enforced (CWE-494): Download of Code Without Integrity Check
Reachability: External · Exploitability: Moderate
Enforce hashed, exact Python dependencies.
apps/intelligence/requirements.txt uses >= constraints and contains no hashes. .github/workflows/security.yml:202-206 installs pip-audit without a version pin and runs it without --require-hashes. Pin every Python requirement with valid hashes and enforce them during installation and auditing, or correct the integrity claim in SECURITY.md:163-169.
🤖 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 `@SECURITY.md` around lines 163 - 169, Align the Python dependency integrity
claim with the implementation: update apps/intelligence/requirements.txt to use
exact versions with valid hashes, pin the pip-audit version, and configure the
security workflow’s installation and audit steps to enforce hashes with
--require-hashes. If this cannot be implemented, revise the SECURITY.md
statement to accurately describe the current Python dependency controls.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/contract-audit.yml (2)
26-26: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSecurity Misconfiguration (CWE-1104)
Reachability: External · Exploitability: Moderate
Correct the
actions/checkoutSHA at.github/workflows/contract-audit.yml:26. Thev4.2.2tag resolves to11bd71901bbe5b1630ceea73d27597364c9af683, notf548e57e544e1ff5a4c46bf1e1b8685f8e4a348a.🤖 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/contract-audit.yml at line 26, Update the actions/checkout reference in the contract-audit workflow to use the SHA 11bd71901bbe5b1630ceea73d27597364c9af683 for the v4.2.2 tag, replacing the incorrect pinned SHA while preserving the existing version comment.Source: Path instructions
72-79: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winSecurity Misconfiguration (CWE-1104)
Reachability: External · Exploitability: Theoretical
Pin
actions/upload-artifactto its v4 release commit.The current SHA is a real commit, but it is not the
v4release commit. Replace both pins withea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2.🤖 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/contract-audit.yml around lines 72 - 79, Update both actions/upload-artifact uses in the contract audit workflow to pin the v4.6.2 release commit ea165f8d65b6e75b540449e92b4886f43607fa02, preserving the existing # v4 version annotation and workflow configuration.Source: Path instructions
🤖 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.
Outside diff comments:
In @.github/workflows/contract-audit.yml:
- Line 26: Update the actions/checkout reference in the contract-audit workflow
to use the SHA 11bd71901bbe5b1630ceea73d27597364c9af683 for the v4.2.2 tag,
replacing the incorrect pinned SHA while preserving the existing version
comment.
- Around line 72-79: Update both actions/upload-artifact uses in the contract
audit workflow to pin the v4.6.2 release commit
ea165f8d65b6e75b540449e92b4886f43607fa02, preserving the existing # v4 version
annotation and workflow configuration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7bed45d1-0788-470a-9d93-c58b2dfd86c0
📒 Files selected for processing (1)
.github/workflows/contract-audit.yml
Syncs the PR branch with upstream dev and fixes the failures the previously-broken CI workflow now exposes on the merge ref: - migrations: renumber colliding prefixes (055/056/060/061/062/082) to 095-101; the only cross-migration dependency (goal_templates used by 083) stays ordered before its consumer - contracts: register_source gained an adapter argument; pass &None in adversarial_tests.rs - intelligence: fix mypy errors (groups annotation, date/datetime typing, FeedbackEntryDict cast, aclose ignore, rating literal cast, getattr for language) - x402 audit: build only contracts/contracts/* (exclude libs) so nester-test-utils is not compiled for wasm
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
.github/workflows/contract-audit.yml (2)
26-36: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSensitive Data Exposure (CWE-200): Exposure of Sensitive Information to an Unauthorized Actor
Reachability: External · Exploitability: Moderate
Disable credentials for the pull-request audit job.
.github/workflows/contract-audit.yml:26-36runs PR-controlled code withX402_PAYMENT_PROOF. A same-repository pull request can exfiltrate this secret.actions/checkoutalso persistsGITHUB_TOKENby default. Setpersist-credentials: falseand declare read-onlypermissions.🤖 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/contract-audit.yml around lines 26 - 36, Update the pull-request audit job around the checkout and audit steps: set actions/checkout’s persist-credentials option to false, and declare least-privilege read-only workflow permissions for the job. Remove X402_PAYMENT_PROOF from the environment passed to scripts/contract-audit.sh while preserving the non-secret audit variables and execution flow.Source: Path instructions
50-54: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSecurity Misconfiguration (CWE-693)
Reachability: External · Exploitability: Moderate
Fail closed when no contract package matches.
If the package-name stream is empty, the loop performs no builds, but lines 58–69 still create the hash file and the step succeeds. Add
pipefail, reject an empty package list, and assert that all intended workspace contracts are selected.🤖 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/contract-audit.yml around lines 50 - 54, Update the contract package-selection and build pipeline in the workflow to fail closed: enable pipefail, reject an empty package-name stream before creating the hash file, and validate that every intended workspace contract is selected while preserving the existing exclusions for test fixtures and tests.Source: Path instructions
SECURITY.md (4)
328-333: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not mutate fields covered by
entry_hash.
detailis part of the hash input at Lines 275-278. Redaction changesdetailbut preservesentry_hash.VerifyChainwill then report a chain break. If verification trusts the preserved hash instead, changes to the redacted content become invisible. Keep the hashed payload immutable and store a redacted projection plus a chained redaction event.🤖 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 `@SECURITY.md` around lines 328 - 333, Update the AuditService.RedactEntry behavior described in SECURITY.md so it never mutates the original entry’s hashed payload, including detail, and does not preserve an entry_hash after changing hashed data. Store the redacted value as a separate projection while retaining the immutable original entry, and continue appending a new chained REDACTION audit entry that records the redaction.
336-340: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDefine
anchor_tx_hashfor file-backed anchors.The MVP writes the hash to a local file, but a file write does not create a transaction hash. Make
anchor_tx_hashnullable for file anchors or define the stable file-record identifier used in its place. Keep the schema and verification contract aligned.🤖 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 `@SECURITY.md` around lines 336 - 340, Clarify the file-backed anchoring contract around AuditService.AnchorLatestEntry by making anchor_tx_hash nullable for local-file anchors, or by defining and documenting a stable file-record identifier used in its place. Align the schema, anchored-state updates, and verification behavior so file anchors do not claim a nonexistent transaction hash.
276-278: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMake the Markdown fences lint-clean.
markdownlint-cli2reports MD031 and MD040 for these blocks. Add blank lines around each fence and specify suitable languages such astext,http, orjson.Also applies to: 303-306, 311-314, 317-319, 322-324
🤖 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 `@SECURITY.md` around lines 276 - 278, Update the fenced code blocks in SECURITY.md, including the block containing the SHA256 expression and the additional referenced blocks, by adding blank lines before and after each fence and assigning an appropriate language tag such as text, http, or json to satisfy markdownlint MD031 and MD040.Source: Linters/SAST tools
346-350: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftSecurity And Privacy (CWE-693)
Reachability: Internal · Exploitability: Difficult
Document the unanchored-tail limitation.
If the latest entry is not externally anchored, a privileged database writer can modify its fields and recompute
entry_hash.VerifyChainhas no laterprev_hashor external-anchor comparison, so it returns success. UpdateSECURITY.md:348and require checkpointing before claiming detection.🤖 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 `@SECURITY.md` around lines 346 - 350, Update the security table near the existing chain-tampering entries to document that modifications to an unanchored latest entry can evade VerifyChain even when entry_hash is recomputed, because no later prev_hash or external anchor validates it. State that detection requires checkpointing or externally anchoring the current tail before claiming this protection.
🤖 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 `@apps/api/migrations/097_add_tamper_evident_audit_fields.up.sql`:
- Around line 1-7: Handle existing audit_logs rows before chained writes read
them: either backfill sequence and hash fields into a valid legacy chain, or
update the audit reader in the query around its ORDER BY sequence DESC to
exclude incomplete rows or place NULLs last. Ensure new audit entries enforce
non-null chain fields after the transition.
- Around line 12-13: The migration files contain multiple index creation
statements in a single file and use regular CREATE INDEX instead of CREATE INDEX
CONCURRENTLY. In
`apps/api/migrations/097_add_tamper_evident_audit_fields.up.sql` at lines 12-13,
split the idx_audit_logs_sequence and idx_audit_logs_entry_hash index creations
into separate migration files, and update each statement to use CREATE INDEX
CONCURRENTLY instead of CREATE INDEX. Similarly, in
`apps/api/migrations/100_reorg_safe_indexer.up.sql` at lines 17-19, extract each
concurrent index creation into its own separate migration file and use CREATE
INDEX CONCURRENTLY. This ensures each concurrent index creation is the sole
statement in its migration file, allowing golang-migrate to execute it outside a
transaction as PostgreSQL requires.
In `@apps/api/migrations/101_add_savings_schedules_onchain_mandate_id.up.sql`:
- Around line 8-10: Split the index operations into a later standalone
concurrent migration: remove the index creation from
apps/api/migrations/101_add_savings_schedules_onchain_mandate_id.up.sql while
retaining its column and comment changes, and add only CREATE INDEX CONCURRENTLY
IF NOT EXISTS there. Move DROP INDEX CONCURRENTLY IF EXISTS to the matching
later down migration, leaving
apps/api/migrations/101_add_savings_schedules_onchain_mandate_id.down.sql to
drop only the column.
In `@apps/intelligence/app/routers/analyze.py`:
- Line 132: Add a language field to the AnalyzeRequest model and ensure the
/analyze handler’s existing language lookup receives the submitted value instead
of defaulting to None. Preserve the current optional behavior when callers omit
language.
---
Outside diff comments:
In @.github/workflows/contract-audit.yml:
- Around line 26-36: Update the pull-request audit job around the checkout and
audit steps: set actions/checkout’s persist-credentials option to false, and
declare least-privilege read-only workflow permissions for the job. Remove
X402_PAYMENT_PROOF from the environment passed to scripts/contract-audit.sh
while preserving the non-secret audit variables and execution flow.
- Around line 50-54: Update the contract package-selection and build pipeline in
the workflow to fail closed: enable pipefail, reject an empty package-name
stream before creating the hash file, and validate that every intended workspace
contract is selected while preserving the existing exclusions for test fixtures
and tests.
In `@SECURITY.md`:
- Around line 328-333: Update the AuditService.RedactEntry behavior described in
SECURITY.md so it never mutates the original entry’s hashed payload, including
detail, and does not preserve an entry_hash after changing hashed data. Store
the redacted value as a separate projection while retaining the immutable
original entry, and continue appending a new chained REDACTION audit entry that
records the redaction.
- Around line 336-340: Clarify the file-backed anchoring contract around
AuditService.AnchorLatestEntry by making anchor_tx_hash nullable for local-file
anchors, or by defining and documenting a stable file-record identifier used in
its place. Align the schema, anchored-state updates, and verification behavior
so file anchors do not claim a nonexistent transaction hash.
- Around line 276-278: Update the fenced code blocks in SECURITY.md, including
the block containing the SHA256 expression and the additional referenced blocks,
by adding blank lines before and after each fence and assigning an appropriate
language tag such as text, http, or json to satisfy markdownlint MD031 and
MD040.
- Around line 346-350: Update the security table near the existing
chain-tampering entries to document that modifications to an unanchored latest
entry can evade VerifyChain even when entry_hash is recomputed, because no later
prev_hash or external anchor validates it. State that detection requires
checkpointing or externally anchoring the current tail before claiming this
protection.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a5fc9600-0051-4a30-aa1b-8d8bf84ccfc6
📒 Files selected for processing (22)
.github/workflows/contract-audit.ymlSECURITY.mdapps/api/migrations/095_add_savings_goal_auto_compound.down.sqlapps/api/migrations/095_add_savings_goal_auto_compound.up.sqlapps/api/migrations/096_create_goal_notification_preferences.down.sqlapps/api/migrations/096_create_goal_notification_preferences.up.sqlapps/api/migrations/097_add_tamper_evident_audit_fields.down.sqlapps/api/migrations/097_add_tamper_evident_audit_fields.up.sqlapps/api/migrations/098_risk_scores.down.sqlapps/api/migrations/098_risk_scores.up.sqlapps/api/migrations/099_chain_submissions.down.sqlapps/api/migrations/099_chain_submissions.up.sqlapps/api/migrations/100_reorg_safe_indexer.down.sqlapps/api/migrations/100_reorg_safe_indexer.up.sqlapps/api/migrations/101_add_savings_schedules_onchain_mandate_id.down.sqlapps/api/migrations/101_add_savings_schedules_onchain_mandate_id.up.sqlapps/intelligence/app/routers/analyze.pyapps/intelligence/app/routers/feedback.pyapps/intelligence/app/routers/ws_chat.pyapps/intelligence/app/services/feedback_store.pyapps/intelligence/app/services/i18n.pypackages/contracts/tests/integration/src/integration/adversarial_tests.rs
| status_code=status.HTTP_400_BAD_REQUEST, detail="prompt is required" | ||
| ) | ||
| language = body.get("language") | ||
| language = getattr(body, "language", None) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify AnalyzeRequest declares a `language` field.
rg -n -A5 'class AnalyzeRequest' apps/intelligence/app/models/recommendation.pyRepository: Suncrest-Labs/nester
Length of output: 338
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- AnalyzeRequest definition ---'
cat -n apps/intelligence/app/models/recommendation.py
printf '%s\n' '--- analyze router context ---'
sed -n '1,210p' apps/intelligence/app/routers/analyze.py
printf '%s\n' '--- language references ---'
rg -n -C3 '\blanguage\b|AnalyzeRequest' apps/intelligence/appRepository: Suncrest-Labs/nester
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- analyze_recommendation definition and language handling ---'
rg -n -A45 -B8 'def analyze_recommendation|async def analyze_recommendation|resolve_language|language=' apps/intelligence/app/services/prometheus.py apps/intelligence/app/services/recommendation_engine.py apps/intelligence/app/services/i18n.py
printf '%s\n' '--- AnalyzeRequest-related tests and API examples ---'
rg -n -C4 'POST.*analyze|/analyze|AnalyzeRequest|language' apps/intelligence/tests tests 2>/dev/null | head -240 || true
printf '%s\n' '--- Pydantic configuration ---'
rg -n -C3 'extra\s*=|ConfigDict|model_config|class Config' apps/intelligence/app apps/intelligence/tests 2>/dev/null | head -160 || trueRepository: Suncrest-Labs/nester
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
import ast
from pathlib import Path
model_path = Path("apps/intelligence/app/models/recommendation.py")
router_path = Path("apps/intelligence/app/routers/analyze.py")
model_tree = ast.parse(model_path.read_text())
request = next(
node for node in model_tree.body
if isinstance(node, ast.ClassDef) and node.name == "AnalyzeRequest"
)
fields = [
node.target.id
for node in request.body
if isinstance(node, ast.AnnAssign)
and isinstance(node.target, ast.Name)
]
print("AnalyzeRequest fields:", fields)
print("declares language:", "language" in fields)
router_tree = ast.parse(router_path.read_text())
analyze_fn = next(
node for node in router_tree.body
if isinstance(node, ast.AsyncFunctionDef) and node.name == "analyze"
)
getattrs = [
node for node in ast.walk(analyze_fn)
if isinstance(node, ast.Call)
and isinstance(node.func, ast.Name)
and node.func.id == "getattr"
]
print("analyze getattr calls:", [
ast.unparse(node) for node in getattrs
])
PYRepository: Suncrest-Labs/nester
Length of output: 276
Add language to AnalyzeRequest. The model declares only prompt, so /analyze always passes None and cannot honor an explicit language preference.
🤖 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 `@apps/intelligence/app/routers/analyze.py` at line 132, Add a language field
to the AnalyzeRequest model and ensure the /analyze handler’s existing language
lookup receives the submitted value instead of defaulting to None. Preserve the
current optional behavior when callers omit language.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
.github/workflows/contract-audit.yml (2)
26-36: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSensitive Data Exposure (CWE-200): Exposure of Sensitive Information to an Unauthorized Actor
Reachability: External · Exploitability: Moderate
Disable credentials for the pull-request audit job.
.github/workflows/contract-audit.yml:26-36runs PR-controlled code withX402_PAYMENT_PROOF. A same-repository pull request can exfiltrate this secret.actions/checkoutalso persistsGITHUB_TOKENby default. Setpersist-credentials: falseand declare read-onlypermissions.🤖 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/contract-audit.yml around lines 26 - 36, Update the pull-request audit job around the checkout and audit steps: set actions/checkout’s persist-credentials option to false, and declare least-privilege read-only workflow permissions for the job. Remove X402_PAYMENT_PROOF from the environment passed to scripts/contract-audit.sh while preserving the non-secret audit variables and execution flow.Source: Path instructions
50-54: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSecurity Misconfiguration (CWE-693)
Reachability: External · Exploitability: Moderate
Fail closed when no contract package matches.
If the package-name stream is empty, the loop performs no builds, but lines 58–69 still create the hash file and the step succeeds. Add
pipefail, reject an empty package list, and assert that all intended workspace contracts are selected.🤖 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/contract-audit.yml around lines 50 - 54, Update the contract package-selection and build pipeline in the workflow to fail closed: enable pipefail, reject an empty package-name stream before creating the hash file, and validate that every intended workspace contract is selected while preserving the existing exclusions for test fixtures and tests.Source: Path instructions
SECURITY.md (4)
328-333: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not mutate fields covered by
entry_hash.
detailis part of the hash input at Lines 275-278. Redaction changesdetailbut preservesentry_hash.VerifyChainwill then report a chain break. If verification trusts the preserved hash instead, changes to the redacted content become invisible. Keep the hashed payload immutable and store a redacted projection plus a chained redaction event.🤖 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 `@SECURITY.md` around lines 328 - 333, Update the AuditService.RedactEntry behavior described in SECURITY.md so it never mutates the original entry’s hashed payload, including detail, and does not preserve an entry_hash after changing hashed data. Store the redacted value as a separate projection while retaining the immutable original entry, and continue appending a new chained REDACTION audit entry that records the redaction.
336-340: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDefine
anchor_tx_hashfor file-backed anchors.The MVP writes the hash to a local file, but a file write does not create a transaction hash. Make
anchor_tx_hashnullable for file anchors or define the stable file-record identifier used in its place. Keep the schema and verification contract aligned.🤖 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 `@SECURITY.md` around lines 336 - 340, Clarify the file-backed anchoring contract around AuditService.AnchorLatestEntry by making anchor_tx_hash nullable for local-file anchors, or by defining and documenting a stable file-record identifier used in its place. Align the schema, anchored-state updates, and verification behavior so file anchors do not claim a nonexistent transaction hash.
276-278: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMake the Markdown fences lint-clean.
markdownlint-cli2reports MD031 and MD040 for these blocks. Add blank lines around each fence and specify suitable languages such astext,http, orjson.Also applies to: 303-306, 311-314, 317-319, 322-324
🤖 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 `@SECURITY.md` around lines 276 - 278, Update the fenced code blocks in SECURITY.md, including the block containing the SHA256 expression and the additional referenced blocks, by adding blank lines before and after each fence and assigning an appropriate language tag such as text, http, or json to satisfy markdownlint MD031 and MD040.Source: Linters/SAST tools
346-350: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftSecurity And Privacy (CWE-693)
Reachability: Internal · Exploitability: Difficult
Document the unanchored-tail limitation.
If the latest entry is not externally anchored, a privileged database writer can modify its fields and recompute
entry_hash.VerifyChainhas no laterprev_hashor external-anchor comparison, so it returns success. UpdateSECURITY.md:348and require checkpointing before claiming detection.🤖 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 `@SECURITY.md` around lines 346 - 350, Update the security table near the existing chain-tampering entries to document that modifications to an unanchored latest entry can evade VerifyChain even when entry_hash is recomputed, because no later prev_hash or external anchor validates it. State that detection requires checkpointing or externally anchoring the current tail before claiming this protection.
🤖 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 `@apps/api/migrations/097_add_tamper_evident_audit_fields.up.sql`:
- Around line 1-7: Handle existing audit_logs rows before chained writes read
them: either backfill sequence and hash fields into a valid legacy chain, or
update the audit reader in the query around its ORDER BY sequence DESC to
exclude incomplete rows or place NULLs last. Ensure new audit entries enforce
non-null chain fields after the transition.
- Around line 12-13: The migration files contain multiple index creation
statements in a single file and use regular CREATE INDEX instead of CREATE INDEX
CONCURRENTLY. In
`apps/api/migrations/097_add_tamper_evident_audit_fields.up.sql` at lines 12-13,
split the idx_audit_logs_sequence and idx_audit_logs_entry_hash index creations
into separate migration files, and update each statement to use CREATE INDEX
CONCURRENTLY instead of CREATE INDEX. Similarly, in
`apps/api/migrations/100_reorg_safe_indexer.up.sql` at lines 17-19, extract each
concurrent index creation into its own separate migration file and use CREATE
INDEX CONCURRENTLY. This ensures each concurrent index creation is the sole
statement in its migration file, allowing golang-migrate to execute it outside a
transaction as PostgreSQL requires.
In `@apps/api/migrations/101_add_savings_schedules_onchain_mandate_id.up.sql`:
- Around line 8-10: Split the index operations into a later standalone
concurrent migration: remove the index creation from
apps/api/migrations/101_add_savings_schedules_onchain_mandate_id.up.sql while
retaining its column and comment changes, and add only CREATE INDEX CONCURRENTLY
IF NOT EXISTS there. Move DROP INDEX CONCURRENTLY IF EXISTS to the matching
later down migration, leaving
apps/api/migrations/101_add_savings_schedules_onchain_mandate_id.down.sql to
drop only the column.
In `@apps/intelligence/app/routers/analyze.py`:
- Line 132: Add a language field to the AnalyzeRequest model and ensure the
/analyze handler’s existing language lookup receives the submitted value instead
of defaulting to None. Preserve the current optional behavior when callers omit
language.
---
Outside diff comments:
In @.github/workflows/contract-audit.yml:
- Around line 26-36: Update the pull-request audit job around the checkout and
audit steps: set actions/checkout’s persist-credentials option to false, and
declare least-privilege read-only workflow permissions for the job. Remove
X402_PAYMENT_PROOF from the environment passed to scripts/contract-audit.sh
while preserving the non-secret audit variables and execution flow.
- Around line 50-54: Update the contract package-selection and build pipeline in
the workflow to fail closed: enable pipefail, reject an empty package-name
stream before creating the hash file, and validate that every intended workspace
contract is selected while preserving the existing exclusions for test fixtures
and tests.
In `@SECURITY.md`:
- Around line 328-333: Update the AuditService.RedactEntry behavior described in
SECURITY.md so it never mutates the original entry’s hashed payload, including
detail, and does not preserve an entry_hash after changing hashed data. Store
the redacted value as a separate projection while retaining the immutable
original entry, and continue appending a new chained REDACTION audit entry that
records the redaction.
- Around line 336-340: Clarify the file-backed anchoring contract around
AuditService.AnchorLatestEntry by making anchor_tx_hash nullable for local-file
anchors, or by defining and documenting a stable file-record identifier used in
its place. Align the schema, anchored-state updates, and verification behavior
so file anchors do not claim a nonexistent transaction hash.
- Around line 276-278: Update the fenced code blocks in SECURITY.md, including
the block containing the SHA256 expression and the additional referenced blocks,
by adding blank lines before and after each fence and assigning an appropriate
language tag such as text, http, or json to satisfy markdownlint MD031 and
MD040.
- Around line 346-350: Update the security table near the existing
chain-tampering entries to document that modifications to an unanchored latest
entry can evade VerifyChain even when entry_hash is recomputed, because no later
prev_hash or external anchor validates it. State that detection requires
checkpointing or externally anchoring the current tail before claiming this
protection.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a5fc9600-0051-4a30-aa1b-8d8bf84ccfc6
📒 Files selected for processing (22)
.github/workflows/contract-audit.ymlSECURITY.mdapps/api/migrations/095_add_savings_goal_auto_compound.down.sqlapps/api/migrations/095_add_savings_goal_auto_compound.up.sqlapps/api/migrations/096_create_goal_notification_preferences.down.sqlapps/api/migrations/096_create_goal_notification_preferences.up.sqlapps/api/migrations/097_add_tamper_evident_audit_fields.down.sqlapps/api/migrations/097_add_tamper_evident_audit_fields.up.sqlapps/api/migrations/098_risk_scores.down.sqlapps/api/migrations/098_risk_scores.up.sqlapps/api/migrations/099_chain_submissions.down.sqlapps/api/migrations/099_chain_submissions.up.sqlapps/api/migrations/100_reorg_safe_indexer.down.sqlapps/api/migrations/100_reorg_safe_indexer.up.sqlapps/api/migrations/101_add_savings_schedules_onchain_mandate_id.down.sqlapps/api/migrations/101_add_savings_schedules_onchain_mandate_id.up.sqlapps/intelligence/app/routers/analyze.pyapps/intelligence/app/routers/feedback.pyapps/intelligence/app/routers/ws_chat.pyapps/intelligence/app/services/feedback_store.pyapps/intelligence/app/services/i18n.pypackages/contracts/tests/integration/src/integration/adversarial_tests.rs
🛑 Comments failed to post (3)
apps/api/migrations/097_add_tamper_evident_audit_fields.up.sql (2)
1-7: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Handle legacy audit rows before enabling chained writes.
audit_logsalready contains rows because this migration alters the table. Those rows retain NULL forsequenceandentry_hash.apps/api/internal/audit/audit.goLines 154-163 selectsORDER BY sequence DESCand scans both fields into non-null Go values. PostgreSQL sorts NULL values first for descending order. The next audit write can therefore fail while scanning a legacy row.Backfill a valid legacy chain before deployment, or update the reader to select only chained rows with
NULLS LAST. Enforce complete chain fields for new rows after the transition.🤖 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 `@apps/api/migrations/097_add_tamper_evident_audit_fields.up.sql` around lines 1 - 7, Handle existing audit_logs rows before chained writes read them: either backfill sequence and hash fields into a valid legacy chain, or update the audit reader in the query around its ORDER BY sequence DESC to exclude incomplete rows or place NULLs last. Ensure new audit entries enforce non-null chain fields after the transition.
12-13: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -eu printf '%s\n' '--- migration files ---' cat -n apps/api/migrations/097_add_tamper_evident_audit_fields.up.sql cat -n apps/api/migrations/100_reorg_safe_indexer.up.sql printf '%s\n' '--- migration tooling and transaction handling ---' rg -n -S --glob '!vendor/**' --glob '!node_modules/**' \ 'migrat|golang-migrate|goose|CREATE INDEX|BeginTx|BEGIN|COMMIT|ExecContext|Exec\(' \ apps/api .github 2>/dev/null | head -300 printf '%s\n' '--- table definitions and write paths ---' rg -n -S --glob '!vendor/**' --glob '!node_modules/**' \ 'audit_logs|processed_events' apps/api | head -300Repository: Suncrest-Labs/nester
Length of output: 48059
🏁 Script executed:
#!/bin/bash set -eu printf '%s\n' '--- startup migration path ---' sed -n '70,125p' apps/api/cmd/api/main.go printf '%s\n' '--- custom migration executor ---' cat -n apps/api/internal/db/migrate.go | sed -n '1,125p' printf '%s\n' '--- migration documentation and precedent ---' cat -n apps/api/migrations/README.md | sed -n '1,90p' cat -n apps/api/migrations/058_add_bank_account_key_version_index.up.sql cat -n apps/api/migrations/070_encrypt_kyc_documents_index.up.sql printf '%s\n' '--- relevant write implementations ---' cat -n apps/api/internal/audit/audit.go | sed -n '195,255p' cat -n apps/api/internal/stellar/reorg_indexer.go | sed -n '220,250p' cat -n apps/api/internal/stellar/indexer.go | sed -n '645,675p' printf '%s\n' '--- migration configuration and callers ---' rg -n -S --glob '*.go' --glob '*.sql' \ 'MigrateUp|MigrateDown|EnableAutoMigrate|migrate\.New|internal/db/migrate|MigrationsDir' apps/apiRepository: Suncrest-Labs/nester
Length of output: 19903
🏁 Script executed:
#!/bin/bash set -eu python3 - <<'PY' from pathlib import Path import re targets = { "apps/api/migrations/097_add_tamper_evident_audit_fields.up.sql": "audit_logs", "apps/api/migrations/100_reorg_safe_indexer.up.sql": "processed_events", } for filename, table in targets.items(): sql = Path(filename).read_text() indexes = re.findall( rf"CREATE\s+(UNIQUE\s+)?INDEX(?:\s+IF\s+NOT\s+EXISTS)?\s+([A-Za-z0-9_]+).*?" rf"\bON\s+{table}\b", sql, re.IGNORECASE | re.DOTALL, ) print(filename) for unique, name in indexes: print(f" {name}: {'UNIQUE ' if unique else ''}INDEX, concurrent={bool(re.search(r'CREATE\\s+(?:UNIQUE\\s+)?INDEX\\s+CONCURRENTLY', sql, re.I))}") migrate_source = Path("apps/api/internal/db/migrate.go").read_text() print("custom MigrateUp begins transaction:", "db.Begin()" in migrate_source) print("custom MigrateUp executes migration through tx:", "tx.Exec(string(content))" in migrate_source) main_source = Path("apps/api/cmd/api/main.go").read_text() print("startup uses golang-migrate:", "migrate.NewWithDatabaseInstance" in main_source) PYRepository: Suncrest-Labs/nester
Length of output: 551
🌐 Web query:
golang-migrate PostgreSQL x-no-tx CREATE INDEX CONCURRENTLY documentation💡 Result:
In golang-migrate, if you need to execute commands that cannot run inside a transaction, such as CREATE INDEX CONCURRENTLY in PostgreSQL, you can use the x-no-tx-wrap query parameter in your database connection string [1]. By setting x-no-tx-wrap=true in your PostgreSQL connection string, you disable the implicit transaction wrapping that golang-migrate otherwise applies to your migration files [1]. Alternatively, for PostgreSQL, the official documentation notes that if you do not wish to use this flag, you must place the CREATE INDEX CONCURRENTLY statement in its own dedicated migration file [2][3]. This ensures the statement is not executed as part of a larger transaction block, as PostgreSQL requires CREATE INDEX CONCURRENTLY to be run outside of a transaction [4][5]. The parameter can typically be applied in the database connection string as follows: postgres://user:password@localhost:5432/database?x-no-tx-wrap=true Note that this configuration option effectively disables the transaction wrapper for the entire migration process associated with that connection, so ensure that all migrations within that scope are compatible with running outside of a transaction [1].
Citations:
- 1: https://docsearch.algolia.com/mcp/docs/repo/golang-migrate/migrate
- 2: https://github.com/golang-migrate/migrate/blob/master/database/postgres/README.md
- 3: https://github.com/golang-migrate/migrate/tree/master/database/postgres
- 4: golang-migrate/migrate#137
- 5: https://squawkhq.com/docs/ban-concurrent-index-creation-in-transaction
🏁 Script executed:
#!/bin/bash set -eu python3 - <<'PY' from pathlib import Path import re targets = { "apps/api/migrations/097_add_tamper_evident_audit_fields.up.sql": "audit_logs", "apps/api/migrations/100_reorg_safe_indexer.up.sql": "processed_events", } for filename, table in targets.items(): sql = Path(filename).read_text() concurrent = bool(re.search( r"CREATE\s+(?:UNIQUE\s+)?INDEX\s+CONCURRENTLY", sql, re.IGNORECASE, )) indexes = re.findall( rf"CREATE\s+(UNIQUE\s+)?INDEX(?:\s+IF\s+NOT\s+EXISTS)?\s+([A-Za-z0-9_]+).*?" rf"\bON\s+{table}\b", sql, re.IGNORECASE | re.DOTALL, ) print(filename) for unique, name in indexes: kind = "UNIQUE " if unique else "" print(f" {name}: {kind}INDEX, concurrent={concurrent}") migrate_source = Path("apps/api/internal/db/migrate.go").read_text() print("custom MigrateUp begins transaction:", "db.Begin()" in migrate_source) print("custom MigrateUp executes migration through tx:", "tx.Exec(string(content))" in migrate_source) main_source = Path("apps/api/cmd/api/main.go").read_text() print("startup uses golang-migrate:", "migrate.NewWithDatabaseInstance" in main_source) PY printf '%s\n' '--- migration URL construction ---' rg -n -S --glob '*.go' --glob '*.env*' \ 'DATABASE_DSN|Database\(\)|NewPostgresDB|dsn|DSN' apps/api/internal apps/api/cmd apps/api/.env.example | head -250Repository: Suncrest-Labs/nester
Length of output: 13026
🌐 Web query:
site:github.com/golang-migrate/migrate database/postgres README x-no-tx-wrap💡 Result:
The parameter x-no-tx-wrap is not a supported feature for the PostgreSQL driver in the golang-migrate/migrate library [1][2]. It is, however, a valid parameter for the SQLite3 and SQLCipher drivers in the same library [3][4][5]. In those drivers, setting x-no-tx-wrap=true disables the default behavior where the library wraps each migration in a transaction, allowing you to run statements that cannot be executed within a transaction (such as certain DDL commands) [3][4]. For the PostgreSQL driver, transactions are typically managed automatically because PostgreSQL supports transactional DDL [1][2]. If you need to run commands that cannot be executed within a transaction—such as CREATE INDEX CONCURRENTLY—the recommended approach in golang-migrate is to place those specific statements into their own separate migration files, as the driver will attempt to wrap migrations in transactions by default [2]. While there have been community discussions and issues requesting a configuration option to disable transactions for PostgreSQL (similar to x-no-tx-wrap) [1], it has not been implemented as a standard URL parameter for that driver. Instead, the PostgreSQL driver includes other specific options such as x-use-transaction, which is used for pg-proxy compatibility [6].
Citations:
- 1: golang-migrate/migrate#641
- 2: https://github.com/golang-migrate/migrate/blob/master/database/postgres/README.md
- 3: golang-migrate/migrate#350
- 4: https://github.com/golang-migrate/migrate/blob/master/database/sqlite3/sqlite3.go
- 5: https://github.com/golang-migrate/migrate/blob/8b9c5f77/database/sqlcipher/sqlcipher.go
- 6: golang-migrate/migrate@257c1fd
🏁 Script executed:
#!/bin/bash set -eu base='https://raw.githubusercontent.com/golang-migrate/migrate/v4.18.2/database/postgres' printf '%s\n' '--- golang-migrate v4.18.2 PostgreSQL driver ---' curl -fsSL "$base/postgres.go" | rg -n -C 5 \ 'NoTx|no-tx|BeginTx|Begin\(|func \(p \*Postgres\) Run|ParseURL' curl -fsSL "$base/README.md" | rg -n -C 4 \ 'transaction|CONCURRENTLY|no-tx|x-use-transaction|URL'Repository: Suncrest-Labs/nester
Length of output: 2558
🏁 Script executed:
#!/bin/bash set -eu url='https://raw.githubusercontent.com/golang-migrate/migrate/v4.18.2/database/postgres/postgres.go' curl -fsSL "$url" | sed -n '40,90p;245,330p' printf '%s\n' '--- postgres README configuration table ---' curl -fsSL 'https://raw.githubusercontent.com/golang-migrate/migrate/v4.18.2/database/postgres/README.md' | sed -n '1,45p'Repository: Suncrest-Labs/nester
Length of output: 6762
Create each index concurrently in a separate migration.
Use
CREATE INDEX CONCURRENTLYfor the indexes at:
apps/api/migrations/097_add_tamper_evident_audit_fields.up.sql:12-13apps/api/migrations/100_reorg_safe_indexer.up.sql:17-19
golang-migratesends each file as one statement by default. PostgreSQL executes multi-statement files transactionally. Keep each concurrent index as the sole statement in its migration.🧰 Tools
🪛 SQLFluff (4.2.2)
[error] 13-13: CREATE INDEX should use CONCURRENTLY to avoid locking the table during the build.
(PG01)
🪛 Squawk (2.61.0)
[warning] 12-12: During normal index creation, table updates are blocked, but reads are still allowed. Use
concurrentlyto avoid blocking writes.(require-concurrent-index-creation)
[warning] 13-13: During normal index creation, table updates are blocked, but reads are still allowed. Use
concurrentlyto avoid blocking writes.(require-concurrent-index-creation)
📍 Affects 2 files
apps/api/migrations/097_add_tamper_evident_audit_fields.up.sql#L12-L13(this comment)apps/api/migrations/100_reorg_safe_indexer.up.sql#L17-L19🤖 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 `@apps/api/migrations/097_add_tamper_evident_audit_fields.up.sql` around lines 12 - 13, The migration files contain multiple index creation statements in a single file and use regular CREATE INDEX instead of CREATE INDEX CONCURRENTLY. In `apps/api/migrations/097_add_tamper_evident_audit_fields.up.sql` at lines 12-13, split the idx_audit_logs_sequence and idx_audit_logs_entry_hash index creations into separate migration files, and update each statement to use CREATE INDEX CONCURRENTLY instead of CREATE INDEX. Similarly, in `apps/api/migrations/100_reorg_safe_indexer.up.sql` at lines 17-19, extract each concurrent index creation into its own separate migration file and use CREATE INDEX CONCURRENTLY. This ensures each concurrent index creation is the sole statement in its migration file, allowing golang-migrate to execute it outside a transaction as PostgreSQL requires.Source: Linters/SAST tools
apps/api/migrations/101_add_savings_schedules_onchain_mandate_id.up.sql (1)
8-10: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Verify that the API migration runner supports standalone non-transactional migrations. fd -t f . apps/api .github | rg '(^|/)(goose|atlas|tern|migrat|dbmate)|go\.mod|Dockerfile|compose' rg -n -i -C 4 \ 'transaction|begin(tx)?|non.transaction|no.transaction|CREATE INDEX CONCURRENTLY|DROP INDEX CONCURRENTLY|migrat' \ apps/api .githubRepository: Suncrest-Labs/nester
Length of output: 50377
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- migration-related files ---' fd -t f . apps/api | rg -i 'migrat|goose|atlas|tern|dbmate|sql' printf '%s\n' '--- migration execution references ---' rg -n -i -C 3 --glob '*.go' --glob '*.md' --glob '*.yml' --glob '*.yaml' --glob 'Dockerfile*' \ 'apply.*migration|migration.*(runner|run|directory)|goose|atlas|tern|dbmate|sql-migrate|golang-migrate|\.up\.sql|\.down\.sql' \ apps/api .github 2>/dev/null | head -n 500 printf '%s\n' '--- target migration files ---' cat -n apps/api/migrations/101_add_savings_schedules_onchain_mandate_id.up.sql cat -n apps/api/migrations/101_add_savings_schedules_onchain_mandate_id.down.sqlRepository: Suncrest-Labs/nester
Length of output: 50378
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- migration runner ---' cat -n apps/api/internal/db/migrate.go printf '%s\n' '--- database initialization ---' cat -n apps/api/internal/db/db.go printf '%s\n' '--- API startup migration call sites ---' rg -n -C 8 'migrate|RunMigrations|Migrations|internal/db' apps/api/cmd/api/main.go apps/api/internal/db printf '%s\n' '--- dependency version ---' rg -n 'golang-migrate|migrate/v4' apps/api/go.mod apps/api/go.sum printf '%s\n' '--- migration transaction guidance ---' cat -n apps/api/migrations/README.md | sed -n '1,90p'Repository: Suncrest-Labs/nester
Length of output: 28731
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- local migration helper usages ---' rg -n -C 5 '\bMigrate(Up|Down)\s*\(' apps/api --glob '*.go' printf '%s\n' '--- golang-migrate PostgreSQL driver transaction behavior ---' tmpdir="$(mktemp -d)" trap 'rm -rf "$tmpdir"' EXIT curl -fsSL \ https://raw.githubusercontent.com/golang-migrate/migrate/v4.18.2/database/postgres/postgres.go \ -o "$tmpdir/postgres.go" rg -n -C 8 'Begin|Commit|Rollback|func \(.*\) Run|Exec\(' "$tmpdir/postgres.go" printf '%s\n' '--- migration statements and transaction-control directives ---' rg -n -i -C 2 \ 'CREATE INDEX|DROP INDEX|BEGIN|COMMIT|ROLLBACK|transaction|concurrently' \ apps/api/migrations/101_add_savings_schedules_onchain_mandate_id.* \ apps/api/migrations/README.mdRepository: Suncrest-Labs/nester
Length of output: 10581
🏁 Script executed:
#!/bin/bash set -euo pipefail tmpdir="$(mktemp -d)" trap 'rm -rf "$tmpdir"' EXIT curl -fsSL \ https://raw.githubusercontent.com/golang-migrate/migrate/v4.18.2/database/postgres/postgres.go \ -o "$tmpdir/postgres.go" printf '%s\n' '--- golang-migrate Run implementation ---' sed -n '260,355p' "$tmpdir/postgres.go" printf '%s\n' '--- complete migration SQL ---' cat -n apps/api/migrations/101_add_savings_schedules_onchain_mandate_id.up.sql cat -n apps/api/migrations/101_add_savings_schedules_onchain_mandate_id.down.sqlRepository: Suncrest-Labs/nester
Length of output: 3624
🌐 Web query:
PostgreSQL CREATE INDEX CONCURRENTLY multiple statements in one query string implicit transaction simple query protocol💡 Result:
In PostgreSQL, you cannot execute the CREATE INDEX CONCURRENTLY command inside a transaction block [1][2]. Attempting to do so will result in an error (SQLSTATE 25001) [1]. This restriction is fundamental because CREATE INDEX CONCURRENTLY functions by building the index in multiple phases, each running in its own separate transaction [1][2]. It cannot operate within an existing, user-managed transaction because it requires the ability to commit its own internal transaction boundaries independently [1][2]. The Simple Query protocol in PostgreSQL handles multiple SQL statements separated by semicolons as a single implicit transaction [3][4][5]. If you send a multi-statement query string containing CREATE INDEX CONCURRENTLY, the entire string is wrapped in an implicit transaction [3][5], causing the command to fail [6][1]. To successfully run CREATE INDEX CONCURRENTLY, you must ensure it is executed in its own command outside of any explicit transaction block [1][2]. This is a common challenge in database migration frameworks, which often wrap migration scripts in a BEGIN/COMMIT block by default [1][2]. To resolve this, you must either: 1. Disable the transactional wrapping for that specific migration or statement [1][2]. 2. Execute the CREATE INDEX CONCURRENTLY command as a standalone query string, ensuring it is not preceded by any open transaction block [1].
Citations:
- 1: https://thesev1database.com/errors/msg-create-index-concurrently-cannot-run-inside-a-transaction-block/
- 2: https://pgfence.com/blog/concurrently-in-a-transaction-silent-footgun/
- 3: https://www.postgresql.org/docs/current/protocol-flow.html
- 4: https://www.postgresql.org/docs/17/protocol-flow.html
- 5: https://github.com/postgres/postgres/blob/e18b0cb7/doc/src/sgml/protocol.sgml
- 6: lib/pq#820
🌐 Web query:
golang-migrate v4.18.2 postgres CREATE INDEX CONCURRENTLY migration transaction behavior💡 Result:
In golang-migrate, migrations using PostgreSQL are by default wrapped in an implicit transaction, which prevents the execution of commands that cannot run inside a transaction block, such as CREATE INDEX CONCURRENTLY [1][2]. To execute CREATE INDEX CONCURRENTLY in a migration, you must ensure it runs outside of an implicit transaction. You can achieve this using one of the following approaches: 1. Use a separate migration file: The most straightforward solution is to isolate the CREATE INDEX CONCURRENTLY statement into its own, single-statement migration file [1][3]. This prevents the driver from attempting to wrap it alongside other statements that might trigger multi-statement transaction behavior [1][2]. 2. Disable implicit transactions: You can disable the driver's default behavior of wrapping migrations in a transaction using the x-no-tx-wrap query parameter in your database connection string (e.g., postgres://user:pass@host:port/dbname?x-no-tx-wrap=true) or via the NoTxWrap configuration option in the driver's instance configuration [4]. Note that when you disable transactions, you become responsible for handling manual rollbacks or cleanup if a migration fails partway through [4]. The driver also supports multi-statement execution via the x-multi-statement=true query parameter or the MultiStatementEnabled configuration option [5][2]. However, this mode still executes statements within a transaction, and does not inherently resolve the restriction against CREATE INDEX CONCURRENTLY [1][5]. Therefore, isolating the command or disabling transaction wrapping remain the recommended practices [1][2].
Citations:
- 1: https://github.com/golang-migrate/migrate/blob/master/database/postgres/README.md
- 2: https://github.com/golang-migrate/migrate/tree/master/database/postgres
- 3: golang-migrate/migrate#284
- 4: https://docsearch.algolia.com/mcp/docs/repo/golang-migrate/migrate
- 5: golang-migrate/migrate#495
Split the index operations into standalone concurrent migrations.
golang-migratesends each migration file as one request. The current files contain multiple statements, so addingCONCURRENTLYin place will fail withcannot run inside a transaction block.
- Keep the column and comment changes in
apps/api/migrations/101_add_savings_schedules_onchain_mandate_id.up.sql.- Move
CREATE INDEX CONCURRENTLY IF NOT EXISTSto a later migration containing only that statement.- Move
DROP INDEX CONCURRENTLY IF EXISTSto the matching down migration. Leave migration 101 down to drop only the column.🧰 Tools
🪛 SQLFluff (4.2.2)
[error] 8-10: CREATE INDEX should use CONCURRENTLY to avoid locking the table during the build.
(PG01)
🪛 Squawk (2.61.0)
[warning] 8-10: During normal index creation, table updates are blocked, but reads are still allowed. Use
concurrentlyto avoid blocking writes.(require-concurrent-index-creation)
📍 Affects 2 files
apps/api/migrations/101_add_savings_schedules_onchain_mandate_id.up.sql#L8-L10(this comment)apps/api/migrations/101_add_savings_schedules_onchain_mandate_id.down.sql#L3-L3🤖 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 `@apps/api/migrations/101_add_savings_schedules_onchain_mandate_id.up.sql` around lines 8 - 10, Split the index operations into a later standalone concurrent migration: remove the index creation from apps/api/migrations/101_add_savings_schedules_onchain_mandate_id.up.sql while retaining its column and comment changes, and add only CREATE INDEX CONCURRENTLY IF NOT EXISTS there. Move DROP INDEX CONCURRENTLY IF EXISTS to the matching later down migration, leaving apps/api/migrations/101_add_savings_schedules_onchain_mandate_id.down.sql to drop only the column.Source: Linters/SAST tools
- risk_service: a 100%-concentrated single-protocol vault (score 32.75) must not be 'low'; narrow the low band from <=33 to <=25
- i18n: French thousands separator is a plain space per the spec, and negative sign leads the formatted amount ('-$42.50' not '$-42.50')
- property_tests: after a successful withdraw, re-baseline prev_price so an empty-vault epoch reset (share_price returns XLM) is not reported as a decrease
…adation refresh_apy_from_adapter cleared the failure streak on every adapter response before the deviation check, so consecutive rejected readings (out of range or beyond the deviation guard) reset-then-recounted to 1 and a source pinned to garbage APY never degraded. Clear the streak only when a reading is committed; rejected readings now accumulate and flip the source to Degraded once the threshold is exceeded.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/internal/services/risk_service.go (1)
565-579: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd test coverage for tier boundary change.
The low/medium boundary moved from 33/34 to 25/26 to fix a classification error: a 100%-concentrated vault scoring 32.75 was incorrectly classified as "low" risk. This is an intentional bug fix per commit 865e120 ("resolve three pre-existing dev test failures").
No test currently covers the
computeTierfunction directly. Add a table-driven test that verifies scores 25, 26, 66, and 67 map to the correct tier and prevents future regressions on this risk classification boundary.🤖 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 `@apps/api/internal/services/risk_service.go` around lines 565 - 579, Add a table-driven unit test for RiskService.computeTier covering boundary scores 25, 26, 66, and 67, asserting they map to low, medium, medium, and high respectively; keep the test focused on preventing regressions in the updated tier boundaries.Source: Path instructions
🤖 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 `@apps/intelligence/app/services/i18n.py`:
- Line 257: Update the French locale entry in _NumberFormat to use a
non-breaking space, such as "\u00a0" or "\u202f", for the thousands separator
instead of a regular space; preserve the existing decimal separator and symbol
formatting options.
In `@packages/contracts/contracts/yield_registry/src/lib.rs`:
- Around line 498-503: Update the ApyConfidence::Unavailable documentation to
state that a successful unavailable response leaves failure_count unchanged,
while only a committed APY reading clears it. Add a regression test covering an
unavailable response and asserting the existing failure count is preserved.
---
Outside diff comments:
In `@apps/api/internal/services/risk_service.go`:
- Around line 565-579: Add a table-driven unit test for RiskService.computeTier
covering boundary scores 25, 26, 66, and 67, asserting they map to low, medium,
medium, and high respectively; keep the test focused on preventing regressions
in the updated tier boundaries.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 87f83d8d-9281-4abb-87f9-957799f927ef
📒 Files selected for processing (4)
apps/api/internal/services/risk_service.goapps/intelligence/app/services/i18n.pypackages/contracts/contracts/yield_registry/src/lib.rspackages/contracts/tests/integration/src/integration/property_tests.rs
| _NUMBER_FORMATS: dict[str, _NumberFormat] = { | ||
| "en": _NumberFormat(".", ",", symbol_prefix=True, symbol_space=False), | ||
| "fr": _NumberFormat(",", " ", symbol_prefix=False, symbol_space=True), | ||
| "fr": _NumberFormat(",", " ", symbol_prefix=False, symbol_space=True), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use a non-breaking space for the French thousands separator.
The French locale now groups digits with a regular space (" "). French typographic convention uses a non-breaking space (U+00A0) or narrow no-break space (U+202F) for this separator. A regular space allows the renderer to wrap the amount across lines, splitting a number like "1 250,00 €" mid-value.
Use "\u00a0" (or "\u202f") instead of " " for the fr group separator.
Proposed fix
- "fr": _NumberFormat(",", " ", symbol_prefix=False, symbol_space=True),
+ "fr": _NumberFormat(",", "\u00a0", symbol_prefix=False, symbol_space=True),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "fr": _NumberFormat(",", " ", symbol_prefix=False, symbol_space=True), | |
| "fr": _NumberFormat(",", "\u00a0", symbol_prefix=False, symbol_space=True), |
🤖 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 `@apps/intelligence/app/services/i18n.py` at line 257, Update the French locale
entry in _NumberFormat to use a non-breaking space, such as "\u00a0" or
"\u202f", for the thousands separator instead of a regular space; preserve the
existing decimal separator and symbol formatting options.
|
@0xDeon I've pushed the fixes for your review feedback (commit
|
) Kept this PR's actual deliverables: the SBOM generation job, the .vulnignore validator wired into the Go audit, SHA-pinned actions, and persist-credentials:false on the k6 checkout. Took the base branch's side on the unrelated changes bundled into this branch, each of which was a regression against it: - risk_service computeTier: this branch's 'overall >= 26' left a gap, so a float score in (25, 26) fell through to "high". The base branch's '< 34' / '< 67' boundaries have no gap. - analyze.py: getattr(body, "language", None) always yields None because AnalyzeRequest has no such field; the base branch takes language as a real query parameter, which is what actually reaches the service. - property_tests.rs: the conflict landed inside the ReportLoss arm, where this branch's withdraw body was misplaced — the Withdraw arm above is already complete. - semgrep step: the base branch replaced the archived action with a pinned pip install, which keeps the reproducibility this PR wanted. - contract-audit: kept the extracted list-contract-packages.py form. - i18n / feedback_store: equivalent forms, took the base branch's. Kept this branch's explanatory comments in yield_registry, where the code was identical on both sides.
- i18n.format_date re-annotated 'parsed' inside the isinstance branch after already declaring it above, which mypy rejects as a redefinition. - feedback.py cast body.rating to FeedbackRating, but the field is already that type, so the cast was redundant. Dropped it and the two imports it was the only user of. - ws_chat.py carried a type: ignore[attr-defined] on stream.aclose() that no longer suppresses anything.
0xDeon
left a comment
0xDeon
left a comment
There was a problem hiding this comment.
LGTM, thanks for your contribution.
2 participants
Closes #867
Summary
Implements comprehensive supply-chain security for the Nester monorepo.
Changes
Summary by CodeRabbit
Security
Bug Fixes
Documentation