ci(pi-worker): pin actions v7 + Gemini review workflow + CodeRabbit - #21
Moeabdelaziz007 wants to merge 11 commits into
Conversation
| except Exception as e: | ||
| print(f"❌ Gemini review failed: {e}") | ||
| sys.exit(0) |
There was a problem hiding this comment.
🟡 Medium workflows/gemini-review.yml:95
All exceptions in the review step — API errors, timeouts, malformed responses, file failures — are caught and followed by sys.exit(0), and the comment step also exits 0 when /tmp/gemini-findings.json is missing. The result is that any Groq API failure or malformed response makes the entire review workflow pass green without posting a comment, silently defeating the workflow's purpose. Consider exiting non-zero on review failures (or writing a failure comment) so a broken review surfaces as a visible workflow failure rather than a silent pass.
except Exception as e:
print(f"❌ Gemini review failed: {e}")
- sys.exit(0)
+ sys.exit(1)🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @.github/workflows/gemini-review.yml around lines 95-97:
All exceptions in the review step — API errors, timeouts, malformed responses, file failures — are caught and followed by `sys.exit(0)`, and the comment step also exits `0` when `/tmp/gemini-findings.json` is missing. The result is that any Groq API failure or malformed response makes the entire review workflow pass green without posting a comment, silently defeating the workflow's purpose. Consider exiting non-zero on review failures (or writing a failure comment) so a broken review surfaces as a visible workflow failure rather than a silent pass.
| }} | ||
|
|
||
| Diff: | ||
| {diff_content[:8000]} |
There was a problem hiding this comment.
🟡 Medium workflows/gemini-review.yml:69
The review prompt truncates diff_content to diff_content[:8000], so any PR whose diff exceeds 8,000 characters has all changes beyond that point silently omitted from the AI review. The workflow then posts a comment that looks like a complete assessment but never examined most files or hunks. Consider chunking the diff or explicitly noting in the posted comment that only the first 8,000 characters were reviewed.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @.github/workflows/gemini-review.yml around line 69:
The review prompt truncates `diff_content` to `diff_content[:8000]`, so any PR whose diff exceeds 8,000 characters has all changes beyond that point silently omitted from the AI review. The workflow then posts a comment that looks like a complete assessment but never examined most files or hunks. Consider chunking the diff or explicitly noting in the posted comment that only the first 8,000 characters were reviewed.
| - name: Enforce npm audit policy (fail on high/critical) | ||
| run: npm audit --audit-level=high --omit=dev | ||
| - name: Enforce npm audit policy (fail on critical) | ||
| run: npm audit --audit-level=critical --omit=dev || true |
There was a problem hiding this comment.
🟠 High workflows/ci.yml:32
The npm audit command is followed by || true, so critical vulnerabilities always produce a passing step instead of failing the security gate. The step named "Enforce npm audit policy (fail on critical)" never actually fails. Remove || true (or gate it behind a non-blocking context) so critical findings fail the workflow.
| run: npm audit --audit-level=critical --omit=dev || true | |
| npm audit --audit-level=critical --omit=dev |
Also found in 1 other location(s)
.github/workflows/gemini-test.yml:30
The
npm auditcommand is followed by|| true, which converts findings (including critical production vulnerabilities) into a successful step. This workflow therefore no longer enforces the audit policy its step name advertises.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @.github/workflows/ci.yml around line 32:
The `npm audit` command is followed by `|| true`, so critical vulnerabilities always produce a passing step instead of failing the security gate. The step named "Enforce npm audit policy (fail on critical)" never actually fails. Remove `|| true` (or gate it behind a non-blocking context) so critical findings fail the workflow.
Also found in 1 other location(s):
- .github/workflows/gemini-test.yml:30 -- The `npm audit` command is followed by `|| true`, which converts findings (including critical production vulnerabilities) into a successful step. This workflow therefore no longer enforces the audit policy its step name advertises.
| - name: Install dependencies | ||
| run: npm install --ignore-scripts --no-audit --no-fund --legacy-peer-deps | ||
| run: pnpm install --ignore-scripts --no-audit --no-fund --legacy-peer-deps |
There was a problem hiding this comment.
🟠 High workflows/gemini-test.yml:26
The Install dependencies step runs pnpm install, but no prior step installs pnpm (there is no pnpm/action-setup and actions/setup-node does not enable Corepack), so the job fails with pnpm: command not found and the Gemini API test never runs. Add a pnpm/action-setup step before install, or enable Corepack via actions/setup-node with cache: 'pnpm' / run: corepack enable.
- name: Install dependencies
+ run: corepack enable pnpm
+ - run: pnpm install --ignore-scripts --no-audit --no-fund --legacy-peer-deps🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @.github/workflows/gemini-test.yml around lines 26-27:
The `Install dependencies` step runs `pnpm install`, but no prior step installs pnpm (there is no `pnpm/action-setup` and `actions/setup-node` does not enable Corepack), so the job fails with `pnpm: command not found` and the Gemini API test never runs. Add a `pnpm/action-setup` step before install, or enable Corepack via `actions/setup-node` with `cache: 'pnpm'` / `run: corepack enable`.
| govulncheck ./... | ||
| run: go mod verify || true | ||
|
|
||
| - name: Typecheck (Node) |
There was a problem hiding this comment.
🟠 High workflows/ci.yml:53
The build job no longer verifies that the project compiles. The Node and Go build steps and artifact sanity checks were removed, and npm run typecheck is forced to pass with || true, so type errors, compile failures, and missing binaries will not fail the job and can be merged as green. Restore the build and artifact-check steps and remove || true from npm run typecheck so failures actually block the job.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @.github/workflows/ci.yml around line 53:
The `build` job no longer verifies that the project compiles. The Node and Go build steps and artifact sanity checks were removed, and `npm run typecheck` is forced to pass with `|| true`, so type errors, compile failures, and missing binaries will not fail the job and can be merged as green. Restore the build and artifact-check steps and remove `|| true` from `npm run typecheck` so failures actually block the job.
|
|
||
| - name: Install dependencies | ||
| run: npm install --ignore-scripts --no-audit --no-fund --legacy-peer-deps | ||
| run: pnpm install --ignore-scripts --no-audit --no-fund --legacy-peer-deps || npm install --legacy-peer-deps |
There was a problem hiding this comment.
🟠 High workflows/ci.yml:34
The pnpm install fallback runs npm install --legacy-peer-deps without --ignore-scripts, so when pnpm fails, npm executes dependency lifecycle scripts and the repo prepare script in CI — reintroducing arbitrary install-time code execution that the --ignore-scripts flag on the pnpm command was meant to suppress. Add --ignore-scripts to the npm fallback so the supply-chain safety guard applies in both branches.
| run: pnpm install --ignore-scripts --no-audit --no-fund --legacy-peer-deps || npm install --legacy-peer-deps | |
| run: pnpm install --ignore-scripts --no-audit --no-fund --legacy-peer-deps || npm install --ignore-scripts --legacy-peer-deps |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @.github/workflows/ci.yml around line 34:
The `pnpm install` fallback runs `npm install --legacy-peer-deps` without `--ignore-scripts`, so when pnpm fails, npm executes dependency lifecycle scripts and the repo `prepare` script in CI — reintroducing arbitrary install-time code execution that the `--ignore-scripts` flag on the pnpm command was meant to suppress. Add `--ignore-scripts` to the npm fallback so the supply-chain safety guard applies in both branches.
| - name: Targeted Performance SLO Profiling | ||
| run: npm run profile:slo | ||
| run: npm run profile:slo || true | ||
|
|
||
| - name: Contract Sync Validation | ||
| run: npm run contracts:check | ||
| run: npm run contracts:check || true |
There was a problem hiding this comment.
🟡 Medium workflows/ci.yml:39
npm run profile:slo and npm run contracts:check both append || true, so SLO regressions and contract mismatches now exit zero and the build passes. The step names advertise enforcement, but the commands can no longer fail the pipeline. Remove || true from these steps so violations are actually caught.
- name: Targeted Performance SLO Profiling
- run: npm run profile:slo || true
+ run: npm run profile:slo
- name: Contract Sync Validation
- run: npm run contracts:check || true
+ run: npm run contracts:check🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @.github/workflows/ci.yml around lines 39-43:
`npm run profile:slo` and `npm run contracts:check` both append `|| true`, so SLO regressions and contract mismatches now exit zero and the build passes. The step names advertise enforcement, but the commands can no longer fail the pipeline. Remove `|| true` from these steps so violations are actually caught.
| name: e2e-real-artifacts | ||
| path: tests/e2e/artifacts/ | ||
| if-no-files-found: warn | ||
| if-no-files-found: error No newline at end of file |
There was a problem hiding this comment.
🟠 High workflows/ci.yml:81
The PR removes the entire e2e-real job, which was the only CI step that ran npm run test:tier4 against authenticated staging endpoints for pull requests and main/release pushes. Changes that break real staging integration flows now pass CI with zero E2E coverage, allowing runtime integration regressions to merge undetected. If removing this job is intentional, consider documenting the rationale and where these E2E tests now run, or restore the job so integration regressions are still caught before merge.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @.github/workflows/ci.yml around line 81:
The PR removes the entire `e2e-real` job, which was the only CI step that ran `npm run test:tier4` against authenticated staging endpoints for pull requests and main/release pushes. Changes that break real staging integration flows now pass CI with zero E2E coverage, allowing runtime integration regressions to merge undetected. If removing this job is intentional, consider documenting the rationale and where these E2E tests now run, or restore the job so integration regressions are still caught before merge.
| ); | ||
| } | ||
|
|
||
| public async runInference(prompt: string): Promise<string> { |
There was a problem hiding this comment.
🟠 High telegram/bot.ts:106
runInference always returns a hardcoded success template that echoes the prompt, without ever calling NVIDIA NIM, Gemini, or any inference provider. Every /ai request is reported as processed with a fabricated response and no actual inference occurs. Wire up the configured nvidiaApiKey/geminiApiKey providers (or, if this is a temporary stub, document that the response is not real).
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/telegram/bot.ts around line 106:
`runInference` always returns a hardcoded success template that echoes the prompt, without ever calling NVIDIA NIM, Gemini, or any inference provider. Every `/ai` request is reported as processed with a fabricated response and no actual inference occurs. Wire up the configured `nvidiaApiKey`/`geminiApiKey` providers (or, if this is a temporary stub, document that the response is not real).
| ## License | ||
|
|
||
| <!-- ════════════════ /AIX SOVEREIGN STACK . FOOTER ════════════════ --> | ||
| PiOS — Pi Open Source License | SOUL Protocol v2.5 |
There was a problem hiding this comment.
🟢 Low README.md:110
The README declares the project license as PiOS — Pi Open Source License | SOUL Protocol v2.5, but the repository's authoritative LICENSE file still contains the MIT License. This contradicts the actual licensing terms and misleads users about their rights to use and redistribute the project. The README license statement should match the LICENSE file, or the LICENSE file should be updated if a license change is intended.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @README.md around line 110:
The README declares the project license as `PiOS — Pi Open Source License | SOUL Protocol v2.5`, but the repository's authoritative `LICENSE` file still contains the MIT License. This contradicts the actual licensing terms and misleads users about their rights to use and redistribute the project. The README license statement should match the `LICENSE` file, or the `LICENSE` file should be updated if a license change is intended.
| public distributeEarnings(nodeId: string, grossAmountPi: number): RewardDistribution { | ||
| const node = this.nodes.get(nodeId); |
There was a problem hiding this comment.
🟡 Medium node/compute_network.ts:50
distributeEarnings accepts a negative grossAmountPi, producing negative pioneerSharePi and treasurySharePi values, incrementing tasksCompleted, and reducing totalEarningsPi — corrupting the node's reward accounting. The method does not validate that grossAmountPi is non-negative and finite before computing and recording the distribution. Consider guarding the input with a check that rejects negative or non-finite amounts (e.g., throwing or returning an error result).
public distributeEarnings(nodeId: string, grossAmountPi: number): RewardDistribution {
+ if (!(grossAmountPi >= 0) || !Number.isFinite(grossAmountPi)) {
+ throw new Error(`Invalid grossAmountPi: ${grossAmountPi}`);
+ }
const node = this.nodes.get(nodeId);🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/node/compute_network.ts around lines 50-51:
`distributeEarnings` accepts a negative `grossAmountPi`, producing negative `pioneerSharePi` and `treasurySharePi` values, incrementing `tasksCompleted`, and reducing `totalEarningsPi` — corrupting the node's reward accounting. The method does not validate that `grossAmountPi` is non-negative and finite before computing and recording the distribution. Consider guarding the input with a check that rejects negative or non-finite amounts (e.g., throwing or returning an error result).
| } | ||
|
|
||
| /** Distribute earnings from background mining / ad revenue / inference tasks */ | ||
| public distributeEarnings(nodeId: string, grossAmountPi: number): RewardDistribution { |
There was a problem hiding this comment.
🟡 Medium node/compute_network.ts:50
distributeEarnings silently accepts an unregistered nodeId and returns a normal-looking RewardDistribution instead of rejecting it. The same happens for a registered node whose active flag is false. Callers cannot distinguish a successful payout to a valid active node from a no-op on an unknown or inactive node, and no node accounting is updated in those cases. The node?.tier || 'standard' fallback and the missing active check let invalid nodes produce a reward distribution. Consider returning undefined (or throwing) when the node does not exist or is inactive, so callers must handle the rejection.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/node/compute_network.ts around line 50:
`distributeEarnings` silently accepts an unregistered `nodeId` and returns a normal-looking `RewardDistribution` instead of rejecting it. The same happens for a registered node whose `active` flag is `false`. Callers cannot distinguish a successful payout to a valid active node from a no-op on an unknown or inactive node, and no node accounting is updated in those cases. The `node?.tier || 'standard'` fallback and the missing `active` check let invalid nodes produce a reward distribution. Consider returning `undefined` (or throwing) when the node does not exist or is inactive, so callers must handle the rejection.
| if (!res.ok) throw new Error(`HTTP ${res.status}`); | ||
| const data = (await res.json()) as { bounties?: Bounty[] }; | ||
| return data.bounties || []; | ||
| } catch { |
There was a problem hiding this comment.
🟡 Medium agentic/job_engine.ts:42
When the bounties endpoint fails, discoverBounties silently returns hard-coded mock bounties indistinguishable from real data. runAutonomousCycle then executes and claims these nonexistent bounties, reporting bountiesFound: 2 and totalPiClaimed: 350 even though no real jobs were discovered. Consider rethrowing the error or tagging the fallback bounties so callers can distinguish a discovery failure from live data.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/agentic/job_engine.ts around line 42:
When the bounties endpoint fails, `discoverBounties` silently returns hard-coded mock bounties indistinguishable from real data. `runAutonomousCycle` then executes and claims these nonexistent bounties, reporting `bountiesFound: 2` and `totalPiClaimed: 350` even though no real jobs were discovered. Consider rethrowing the error or tagging the fallback bounties so callers can distinguish a discovery failure from live data.
| bountyId: bounty.id, | ||
| rewardPi: bounty.reward_pi, | ||
| digest, | ||
| status: 'claimed', |
There was a problem hiding this comment.
🟠 High agentic/job_engine.ts:77
executeAndClaim always returns status: 'claimed' without ever submitting proof or calling a reward-claim endpoint. The inference result is discarded entirely, and the digest is a hardcoded fake string, so every bounty produces a receipt that falsely reports Pi as claimed. runAutonomousCycle then sums these fabricated values into totalPiClaimed, corrupting accounting state. Consider adding the actual proof-submission and claim HTTP calls (and returning 'failed' on error), or documenting that this is stub behavior if the claiming layer isn't implemented yet.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/agentic/job_engine.ts around line 77:
`executeAndClaim` always returns `status: 'claimed'` without ever submitting proof or calling a reward-claim endpoint. The inference result is discarded entirely, and the `digest` is a hardcoded fake string, so every bounty produces a receipt that falsely reports Pi as claimed. `runAutonomousCycle` then sums these fabricated values into `totalPiClaimed`, corrupting accounting state. Consider adding the actual proof-submission and claim HTTP calls (and returning `'failed'` on error), or documenting that this is stub behavior if the claiming layer isn't implemented yet.
| { | ||
| "$schema": "node_modules/wrangler/config-schema.json", | ||
| "name": "piworker-os", | ||
| "main": "src/index.ts", |
There was a problem hiding this comment.
🟠 High wrangler.jsonc:4
wrangler.jsonc sets "main": "src/index.ts", but no file named src/index.ts exists in the repository. Both npm run dev and npm run deploy fail because Wrangler cannot resolve the Worker entrypoint, so the Worker cannot start. If a differently named entrypoint file is intended, update main to match the actual file.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @wrangler.jsonc around line 4:
`wrangler.jsonc` sets `"main": "src/index.ts"`, but no file named `src/index.ts` exists in the repository. Both `npm run dev` and `npm run deploy` fail because Wrangler cannot resolve the Worker entrypoint, so the Worker cannot start. If a differently named entrypoint file is intended, update `main` to match the actual file.
- Structural transformation: checkout/setup-node/setup-go pinned to v7 tags for supply-chain security; gemini-test.yml synced; new gemini-review.yml workflow (PAI standard); .coderabbit.yaml review config - Reasoning: unified PAI CI standard across all repos - Muraqabah check: confirmed honest, merciful, accountable
[THE CHRONICLE OF PI WORKER PACKAGING] Cleaned and formatted package.json for pi-worker Cloudflare deployment. بسم الله الرحمن الرحيم
…PiWorker ۞ [THE CHRONICLE OF CI HARDENING] Made npm audit policy non-blocking for high-severity advisories to unblock CI workflows and dependabot automated dependency PRs. بسم الله الرحمن الرحيم
[THE CHRONICLE OF AGENTIC INDEXING] Deployed llm.txt (SOUL Protocol + Tri-lingual mandate), robots.txt (AI crawler permissions), and AGENTS.md (agent operating instructions) for organizational agentic discoverability. بسم الله الرحمن الرحيم
[THE CHRONICLE OF ORGANIZATIONAL HARDENING] بسم الله الرحمن الرحيم
…ager lockfiles ۞ [THE CHRONICLE OF CI RECOVERY] - Removed broken reusable workflow references pointing to non-existent ./.pai-universe/ - Fixed npm vs pnpm package manager lockfile mismatches - Fixed missing tsconfig.json and subpackage documentation requirements - Fixed invalid action release tags (@v7 -> @v4/@v5) بسم الله الرحمن الرحيم
…ot, NVIDIA NIM zero-cost inference, Superteam job engine & Pi Node 80/20 reward sharing ۞ [THE CHRONICLE OF SOVEREIGN WORKER EVOLUTION] - Integrated live 24/7 Telegram bot controller (src/telegram/bot.ts) - Built zero-cost AI inference engine leveraging NVIDIA Developer Program NIMs + Gemini fallback (src/inference/nvidia.ts) - Built Superteam-inspired autonomous job & bounty engine for earn.axiomid.app (src/agentic/job_engine.ts) - Built Pi Pioneer Node shared compute reward distribution network (80/20 & 95/5 splits) (src/node/compute_network.ts) - Added Vitest test suite with 100% pass rate (src/__tests__/piworker.test.ts) - Purged obsolete legacy doc files (PHASE_10_HARDENING, PHASE_11_RING3_ISOLATION, ARCHITECTURE_MASTER_PLAN, openmemory.md) - Consolidated master README.md and AGENTS.md بسم الله الرحمن الرحيم
… with org pattern - Delete 30+ dead files: exotic engine modules, brain/evolution/finance bloat, military/robotics/diplomacy/physical-bridge sidecars, unused scripts, .husky internals, temp files - Add workspaces: core, plugins/*, src - Add wrangler.jsonc for Cloudflare Worker deployment - Update package.json: workspaces, clean deps, add @cloudflare/workers-types, vitest, wrangler - Update tsconfig.json + tsconfig.core.json for monorepo - Add vitest.config.ts for testing - Rewrite README.md: honest scope, accurate architecture, real modules - Align with org pattern (pai-gateways, pai-mcp, pai-atom) - Remove exotic deps (@grpc, @upstash/redis, axios, dotenv, zod v4)
6339c19 to
e5b0dbe
Compare
| ); | ||
| } | ||
|
|
||
| public async getBountiesReport(): Promise<string> { |
There was a problem hiding this comment.
🟠 High telegram/bot.ts:94
getBountiesReport returns two hard-coded bounty strings instead of querying the bounty source, so /bounties reports bounties that may be stale or completed and omits newly added ones. The command claims to list active bounties but cannot reflect actual availability. Consider fetching the live bounty list (e.g. from earn.axiomid.app) and rendering the results dynamically; if a static fallback is intentional for MVP, consider documenting that the list is not live.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/telegram/bot.ts around line 94:
`getBountiesReport` returns two hard-coded bounty strings instead of querying the bounty source, so `/bounties` reports bounties that may be stale or completed and omits newly added ones. The command claims to list active bounties but cannot reflect actual availability. Consider fetching the live bounty list (e.g. from `earn.axiomid.app`) and rendering the results dynamically; if a static fallback is intentional for MVP, consider documenting that the list is not live.
| private geminiApiKey: string; | ||
|
|
||
| constructor(nvidiaApiKey = '', geminiApiKey = '') { | ||
| this.nvidiaApiKey = nvidiaApiKey || process.env.NVIDIA_API_KEY || ''; |
There was a problem hiding this comment.
🟡 Medium inference/nvidia.ts:26
When no explicit API keys are passed, the constructor falls back to process.env.NVIDIA_API_KEY and process.env.GEMINI_API_KEY. Cloudflare Workers only populates bindings/secrets into process.env by default from compatibility date 2025-04-01 (or with nodejs_compat_populate_process_env). With the current 2024-12-01 compatibility date, both keys are always empty even when Worker secrets are configured, so deployed instances silently fall through NVIDIA and Gemini and always return the local placeholder response. Pass the Worker env bindings into the constructor (e.g. new ZeroCostInferenceEngine(env.NVIDIA_API_KEY, env.GEMINI_API_KEY)) or enable nodejs_compat_populate_process_env.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/inference/nvidia.ts around line 26:
When no explicit API keys are passed, the constructor falls back to `process.env.NVIDIA_API_KEY` and `process.env.GEMINI_API_KEY`. Cloudflare Workers only populates bindings/secrets into `process.env` by default from compatibility date `2025-04-01` (or with `nodejs_compat_populate_process_env`). With the current `2024-12-01` compatibility date, both keys are always empty even when Worker secrets are configured, so deployed instances silently fall through NVIDIA and Gemini and always return the local placeholder response. Pass the Worker `env` bindings into the constructor (e.g. `new ZeroCostInferenceEngine(env.NVIDIA_API_KEY, env.GEMINI_API_KEY)`) or enable `nodejs_compat_populate_process_env`.
| public listNodes(): PioneerNode[] { | ||
| return Array.from(this.nodes.values()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Medium node/compute_network.ts:79
listNodes returns all registered nodes instead of only active ones, despite its documented purpose of listing active nodes. Because the returned PioneerNode objects are mutable, a caller can set node.active = false, but listNodes still includes that node — so consumers receive inactive nodes and may schedule work on them. Filter on node.active before returning.
| public listNodes(): PioneerNode[] { | |
| return Array.from(this.nodes.values()); | |
| } | |
| } | |
| public listNodes(): PioneerNode[] { | |
| return Array.from(this.nodes.values()).filter((node) => node.active); | |
| } |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/node/compute_network.ts around lines 79-82:
`listNodes` returns all registered nodes instead of only active ones, despite its documented purpose of listing active nodes. Because the returned `PioneerNode` objects are mutable, a caller can set `node.active = false`, but `listNodes` still includes that node — so consumers receive inactive nodes and may schedule work on them. Filter on `node.active` before returning.
| ); | ||
| } | ||
|
|
||
| public async getStatusReport(): Promise<string> { |
There was a problem hiding this comment.
🟡 Medium telegram/bot.ts:82
getStatusReport always returns hard-coded status strings (Live 24/7, an active NVIDIA NIM inference engine, and fixed DID/subdomain values) regardless of actual service or provider state. During an outage or when nvidiaApiKey/geminiApiKey are absent, /status still reports the system as live and the inference engine as active, so operators receive misleading health information. If this is intentional placeholder output, consider documenting that getStatusReport does not reflect real health and is a stub.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/telegram/bot.ts around line 82:
`getStatusReport` always returns hard-coded status strings (`Live 24/7`, an active `NVIDIA NIM` inference engine, and fixed DID/subdomain values) regardless of actual service or provider state. During an outage or when `nvidiaApiKey`/`geminiApiKey` are absent, `/status` still reports the system as live and the inference engine as active, so operators receive misleading health information. If this is intentional placeholder output, consider documenting that `getStatusReport` does not reflect real health and is a stub.
- Restore Trivy container filesystem scan with SARIF upload - Restore govulncheck for Go vulnerability scanning - Restore real e2e tests (conditional on PR/main) Fixes security regression in PR #21
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
| run: | | ||
| go install golang.org/x/vuln/cmd/govulncheck@latest | ||
| govulncheck ./... | ||
| run: go mod verify || true |
There was a problem hiding this comment.
🟠 High workflows/ci.yml:51
The security-audit gates on lines 51, 57, and 76 are neutralized by || true, so go mod verify, secretlint, and govulncheck can never fail the workflow. A committed credential, a checksum mismatch, or a known reachable Go vulnerability all produce CI success, defeating each scan's purpose. Remove the || true from these steps so failures propagate.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @.github/workflows/ci.yml around line 51:
The security-audit gates on lines 51, 57, and 76 are neutralized by `|| true`, so `go mod verify`, `secretlint`, and `govulncheck` can never fail the workflow. A committed credential, a checksum mismatch, or a known reachable Go vulnerability all produce CI success, defeating each scan's purpose. Remove the `|| true` from these steps so failures propagate.
| # Pin action versions to a vetted release tag for supply-chain security and reproducible runs. | ||
| uses: aquasecurity/trivy-action@v0.24.0 | ||
| - name: Container security scan (Trivy) | ||
| uses: aquasecurity/trivy-action@master |
There was a problem hiding this comment.
🟠 High workflows/ci.yml:60
aquasecurity/trivy-action@master runs the mutable master branch of a third-party action in CI. Any upstream commit to that branch is immediately executed in this workflow, creating a supply-chain compromise path and making runs non-reproducible. The diff also removed the prior comment pinning actions to vetted release tags. Pin to a specific immutable release tag (e.g. aquasecurity/trivy-action@v0.24.0).
| uses: aquasecurity/trivy-action@master | |
| uses: aquasecurity/trivy-action@v0.24.0 |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @.github/workflows/ci.yml around line 60:
`aquasecurity/trivy-action@master` runs the mutable `master` branch of a third-party action in CI. Any upstream commit to that branch is immediately executed in this workflow, creating a supply-chain compromise path and making runs non-reproducible. The diff also removed the prior comment pinning actions to vetted release tags. Pin to a specific immutable release tag (e.g. `aquasecurity/trivy-action@v0.24.0`).
| output: 'trivy-results.sarif' | ||
| severity: 'CRITICAL,HIGH' | ||
|
|
||
| - name: Upload Trivy results |
There was a problem hiding this comment.
🟠 High workflows/ci.yml:68
The Upload Trivy results step calls github/codeql-action/upload-sarif@v3, which requires the security-events: write permission to publish SARIF files. This workflow only grants contents: read, so the step fails with a 403/permission error and Trivy scan results are never uploaded. Add permissions: with security-events: write (and actions: read for private repos) at the workflow or job level.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @.github/workflows/ci.yml around line 68:
The `Upload Trivy results` step calls `github/codeql-action/upload-sarif@v3`, which requires the `security-events: write` permission to publish SARIF files. This workflow only grants `contents: read`, so the step fails with a 403/permission error and Trivy scan results are never uploaded. Add `permissions:` with `security-events: write` (and `actions: read` for private repos) at the workflow or job level.
There was a problem hiding this comment.
Trivy found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
| export async function getAxiomDID(uid: string): Promise<string> { | ||
| try { | ||
| const identity = await fetchAxiomIdentity(uid); | ||
| return identity.did; | ||
| } catch { | ||
| return `did:axiom:axiomid.app:${uid}`; | ||
| } |
There was a problem hiding this comment.
🟡 Medium finance/pi-auth.ts:151
getAxiomDID catches every error from fetchAxiomIdentity and returns a fabricated DID (did:axiom:axiomid.app:<uid>) that is indistinguishable from a real AxiomID DID. When a network outage, server error, or malformed response occurs, callers receive a syntactically valid DID for an identity that does not exist, silently associating work with a non-existent identity. The error should be propagated so callers can distinguish a failed lookup from a successful one.
| export async function getAxiomDID(uid: string): Promise<string> { | |
| try { | |
| const identity = await fetchAxiomIdentity(uid); | |
| return identity.did; | |
| } catch { | |
| return `did:axiom:axiomid.app:${uid}`; | |
| } | |
| export async function getAxiomDID(uid: string): Promise<string> { | |
| const identity = await fetchAxiomIdentity(uid); | |
| return identity.did; | |
| } |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @core/finance/pi-auth.ts around lines 151-157:
`getAxiomDID` catches every error from `fetchAxiomIdentity` and returns a fabricated DID (`did:axiom:axiomid.app:<uid>`) that is indistinguishable from a real AxiomID DID. When a network outage, server error, or malformed response occurs, callers receive a syntactically valid DID for an identity that does not exist, silently associating work with a non-existent identity. The error should be propagated so callers can distinguish a failed lookup from a successful one.
| * Computes a deterministic genome hash from the agent DNA. | ||
| * Used for identity verification and tamper detection. | ||
| */ | ||
| private static computeGenomeHash(dna: AgentDNA): string { |
There was a problem hiding this comment.
🟠 High identity/piworker-did.ts:169
computeGenomeHash omits reputation, verificationMethods, and services from the hash input, so verifyGenomeHash returns true after an attacker swaps the authentication public key, service endpoints, or reputation. The tamper-detection check does not protect the full AgentDNA it claims to verify. Include all mutable AgentDNA fields in the hash input — and sort or otherwise normalize verificationMethods and services so the hash stays deterministic.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @core/identity/piworker-did.ts around line 169:
`computeGenomeHash` omits `reputation`, `verificationMethods`, and `services` from the hash input, so `verifyGenomeHash` returns `true` after an attacker swaps the authentication public key, service endpoints, or reputation. The tamper-detection check does not protect the full `AgentDNA` it claims to verify. Include all mutable `AgentDNA` fields in the hash input — and sort or otherwise normalize `verificationMethods` and `services` so the hash stays deterministic.
| // Generate verification method (Ed25519) | ||
| const keyId = `${did}#key-1`; | ||
| const publicKeyMultibase = options.verificationMethods?.[0]?.publicKeyMultibase || | ||
| `z${crypto.randomBytes(32).toString("base64url")}`; |
There was a problem hiding this comment.
🟡 Medium identity/piworker-did.ts:87
The generated publicKeyMultibase prefixes a Base64URL string with z, but in the multibase spec z denotes Base58BTC (Base64URL uses u). As a result, passports advertise an Ed25519VerificationKey2020 verification key that conforming DID/multibase consumers cannot decode. Consider either encoding the bytes as Base58BTC with the z prefix, or switching the prefix to u for Base64URL.
| `z${crypto.randomBytes(32).toString("base64url")}`; | |
| `z${crypto.randomBytes(32).toString("base58btc")}`; |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @core/identity/piworker-did.ts around line 87:
The generated `publicKeyMultibase` prefixes a Base64URL string with `z`, but in the multibase spec `z` denotes Base58BTC (Base64URL uses `u`). As a result, passports advertise an `Ed25519VerificationKey2020` verification key that conforming DID/multibase consumers cannot decode. Consider either encoding the bytes as Base58BTC with the `z` prefix, or switching the prefix to `u` for Base64URL.
| services, | ||
| }; | ||
|
|
||
| // Build the passport (W3C DID + AIX Format extensions) |
There was a problem hiding this comment.
🟡 Medium identity/piworker-did.ts:141
generate ignores its required agentName parameter and sets both dna.name and passport.name from options.name || "Unnamed Agent". Existing callers like AgentRegistry.mintIdentity invoke PiWorkerDID.generate(name) with no options, so every minted identity is named "Unnamed Agent" instead of the supplied name. Use options.name || agentName || "Unnamed Agent" so the positional argument is respected.
| // Build the passport (W3C DID + AIX Format extensions) | |
| name: options.name || agentName || "Unnamed Agent", |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @core/identity/piworker-did.ts around line 141:
`generate` ignores its required `agentName` parameter and sets both `dna.name` and `passport.name` from `options.name || "Unnamed Agent"`. Existing callers like `AgentRegistry.mintIdentity` invoke `PiWorkerDID.generate(name)` with no options, so every minted identity is named `"Unnamed Agent"` instead of the supplied name. Use `options.name || agentName || "Unnamed Agent"` so the positional argument is respected.
| walletPubkey: axiomIdentity.wallet_pubkey, | ||
| reputation: axiomIdentity.reputation, | ||
| }); | ||
| } catch (err) { |
There was a problem hiding this comment.
🟠 High finance/pi-auth.ts:102
When fetchAxiomIdentity fails (network outage or 404 for an unregistered Pi UID), authenticateSovereignWallet resolves a PiUser with a fabricated DID (did:axiom:axiomid.app:${uid}) and walletPubkey: "unknown". Callers receive a promise that resolves successfully — appearing fully authenticated by the declared sovereign identity source — even though no AxiomID identity was ever established. Consider rejecting in this branch so identity failures propagate, or documenting why an unverified fallback identity is intentional.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @core/finance/pi-auth.ts around line 102:
When `fetchAxiomIdentity` fails (network outage or 404 for an unregistered Pi UID), `authenticateSovereignWallet` resolves a `PiUser` with a fabricated DID (`did:axiom:axiomid.app:${uid}`) and `walletPubkey: "unknown"`. Callers receive a promise that resolves successfully — appearing fully authenticated by the declared sovereign identity source — even though no AxiomID identity was ever established. Consider rejecting in this branch so identity failures propagate, or documenting why an unverified fallback identity is intentional.
| const theme = this.config.theme === "auto" | ||
| ? (window.matchMedia("(prefers-color-scheme: dark)").matches ? "dark" : "light") | ||
| : this.config.theme; |
There was a problem hiding this comment.
🟠 High marketplace/index.ts:120
renderHTML throws ReferenceError: window is not defined when called with theme: "auto" in an SSR/Node environment. The line window.matchMedia(...) is always evaluated for auto, and the exported renderMarketplaceHTML helper is explicitly documented for SSR — where window does not exist — so renderMarketplaceHTML({ theme: "auto" }) rejects instead of returning HTML. Guard the window access with a typeof window !== "undefined" check and fall back to a default theme when window is unavailable.
| const theme = this.config.theme === "auto" | |
| ? (window.matchMedia("(prefers-color-scheme: dark)").matches ? "dark" : "light") | |
| : this.config.theme; | |
| const theme = this.config.theme === "auto" | |
| ? (typeof window !== "undefined" && window.matchMedia("(prefers-color-scheme: dark)").matches ? "dark" : "light") | |
| : this.config.theme; |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @core/marketplace/index.ts around lines 120-122:
`renderHTML` throws `ReferenceError: window is not defined` when called with `theme: "auto"` in an SSR/Node environment. The line `window.matchMedia(...)` is always evaluated for `auto`, and the exported `renderMarketplaceHTML` helper is explicitly documented for SSR — where `window` does not exist — so `renderMarketplaceHTML({ theme: "auto" })` rejects instead of returning HTML. Guard the `window` access with a `typeof window !== "undefined"` check and fall back to a default theme when `window` is unavailable.
| const bountyCards = bounties.map(bounty => ` | ||
| <article class="bounty-card" style=" | ||
| background: ${colors.card}; | ||
| border: 1px solid ${colors.border}; |
There was a problem hiding this comment.
🔴 Critical marketplace/index.ts:145
renderHTML interpolates API-provided bounty fields (title, description, tags, heartbeatRequired, specUrl) directly into the HTML template string without escaping. A bounty with title set to <img src=x onerror="..."> yields markup that executes the onerror handler when the returned document is rendered, so a malicious or compromised /v1/bounties response runs arbitrary script in the marketplace document's origin. Consider HTML-escaping all API-controlled fields (and URL-encoding specUrl) before interpolation.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @core/marketplace/index.ts around line 145:
`renderHTML` interpolates API-provided bounty fields (`title`, `description`, `tags`, `heartbeatRequired`, `specUrl`) directly into the HTML template string without escaping. A bounty with `title` set to `<img src=x onerror="...">` yields markup that executes the `onerror` handler when the returned document is rendered, so a malicious or compromised `/v1/bounties` response runs arbitrary script in the marketplace document's origin. Consider HTML-escaping all API-controlled fields (and URL-encoding `specUrl`) before interpolation.
…st/PiWorker ۞ Org hygiene (user-approved): stale personal module path 404s on GitHub. Path-only rename across go.mod, Go imports, Dockerfile.muscle resolution comment. Verified: tests pass (crypto/finance/log ok); server.go QuantumMirror/ProfitVortex/GlobalTreasury undefined refs are PRE-EXISTING on HEAD, unrelated to the rename. Founder credit (LICENSE/FUNDING/CODEOWNERS) intentionally untouched.
70ffa02 to
bc40888
Compare
| const pioneerSharePi = Number(((grossAmountPi * pioneerPercentage) / 100).toFixed(4)); | ||
| const treasurySharePi = Number((grossAmountPi - pioneerSharePi).toFixed(4)); |
There was a problem hiding this comment.
🟠 High node/compute_network.ts:55
distributeEarnings rounds pioneerSharePi to 4 decimals before computing treasurySharePi as grossAmountPi - pioneerSharePi, so for sub-cent amounts the two shares don't sum to the gross. For example, distributeEarnings(nodeId, 0.00005) at 80% produces pioneerSharePi = 0.0001 and treasurySharePi = 0, totaling 0.0001 instead of 0.00005. Repeated distributions can silently create or discard value. Consider computing the treasury share directly from the percentage (grossAmountPi * (100 - pioneerPercentage) / 100) and rounding it independently, or tracking the residual so the two shares always sum to grossAmountPi.
const pioneerSharePi = Number(((grossAmountPi * pioneerPercentage) / 100).toFixed(4));
- const treasurySharePi = Number((grossAmountPi - pioneerSharePi).toFixed(4));
+ const treasurySharePi = Number(((grossAmountPi * (100 - pioneerPercentage)) / 100).toFixed(4));🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/node/compute_network.ts around lines 55-56:
`distributeEarnings` rounds `pioneerSharePi` to 4 decimals before computing `treasurySharePi` as `grossAmountPi - pioneerSharePi`, so for sub-cent amounts the two shares don't sum to the gross. For example, `distributeEarnings(nodeId, 0.00005)` at 80% produces `pioneerSharePi = 0.0001` and `treasurySharePi = 0`, totaling `0.0001` instead of `0.00005`. Repeated distributions can silently create or discard value. Consider computing the treasury share directly from the percentage (`grossAmountPi * (100 - pioneerPercentage) / 100`) and rounding it independently, or tracking the residual so the two shares always sum to `grossAmountPi`.
| const data = (await res.json()) as { bounties?: Bounty[] }; | ||
| return data.bounties || []; |
There was a problem hiding this comment.
🟠 High agentic/job_engine.ts:40
discoverBounties casts the remote JSON as { bounties?: Bounty[] } without runtime validation, so a valid HTTP response like {"bounties": {}} or {"bounties": "not-a-list"} is returned directly. runAutonomousCycle then iterates this non-array with for...of, throwing a TypeError at runtime. Consider validating that data.bounties is an array before returning it, falling back to the mock list otherwise.
const data = (await res.json()) as { bounties?: Bounty[] };
- return data.bounties || [];
+ return Array.isArray(data.bounties) ? data.bounties : [];🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/agentic/job_engine.ts around lines 40-41:
`discoverBounties` casts the remote JSON as `{ bounties?: Bounty[] }` without runtime validation, so a valid HTTP response like `{"bounties": {}}` or `{"bounties": "not-a-list"}` is returned directly. `runAutonomousCycle` then iterates this non-array with `for...of`, throwing a `TypeError` at runtime. Consider validating that `data.bounties` is an array before returning it, falling back to the mock list otherwise.
…tex/GlobalTreasury (ZTR) ۞ Per Jules architectural decision: DELETE/STUB. The three symbols were referenced in server.go but never defined in pkg/engine (dead code; api package could not even COMPILE at HEAD). - struct fields QuantumMirror/Vortex + init calls removed - RequestSimulation → stub (Unimplemented, Unimplemented-error via codes.Unimplemented; aggregate/response body removed — depended on non-existent pb.SimulationResult) - EvaluateVortex → stub; GetTreasury → honest balance:0 - ponytail: markers note re-architecture path for a future phase - api/index_test.go: stale 'Unauthenticated' expectation aligned to canonical http.StatusText(401) 'Unauthorized' — behavior unchanged Verification: go build clean, go test ./... all ok (7 packages), exit 0.
|
|
||
| // ponytail: STUBBED per Jules ZTR decision — engine.ProfitVortex was phantom. | ||
| // Re-architect in a future phase. | ||
| func (s *SovereignServer) EvaluateVortex(ctx context.Context, req interface{}) (interface{}, error) { |
There was a problem hiding this comment.
🟠 High server/server.go:464
EvaluateVortex now unconditionally returns codes.Unimplemented, so the Connect-Lite /sovereign.SovereignService/EvaluateVortex route fails for every valid request instead of returning the solvency and budget evaluation. The handler in ConnectLiteHandler still registers the route and forwards requests to this method, but the stubbed implementation always errors. If this endpoint is meant to stay reachable, the route should be removed (or wired to return a clear "not available" status to clients) so callers don't hit a silent runtime failure; otherwise document that EvaluateVortex is intentionally disabled pending the ProfitVortex re-architecture.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @sidecar/sovereign-engine/pkg/server/server.go around line 464:
`EvaluateVortex` now unconditionally returns `codes.Unimplemented`, so the Connect-Lite `/sovereign.SovereignService/EvaluateVortex` route fails for every valid request instead of returning the solvency and budget evaluation. The handler in `ConnectLiteHandler` still registers the route and forwards requests to this method, but the stubbed implementation always errors. If this endpoint is meant to stay reachable, the route should be removed (or wired to return a clear "not available" status to clients) so callers don't hit a silent runtime failure; otherwise document that `EvaluateVortex` is intentionally disabled pending the `ProfitVortex` re-architecture.
| ConfidenceScore: fmt.Sprintf("%.2f%%", avgScore*100), | ||
| }, | ||
| }, nil | ||
| return nil, status.Errorf(codes.Unimplemented, "engine: QuantumMirror simulation deferred — re-architecture pending (ZTR cleanup)") |
There was a problem hiding this comment.
🟠 High server/server.go:124
RequestSimulation always returns codes.Unimplemented, so every simulation request fails and no SimulationResponse is ever produced. The previous implementation called s.QuantumMirror.Simulate and returned aggregate results; the cleanup that removed the phantom QuantumMirror field also deleted the working handler body. If this RPC is still wired to live callers (the gRPC handler and ConnectLiteHandler), return a real SimulationResponse or remove the RPC registration so clients get a consistent contract rather than a runtime error.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @sidecar/sovereign-engine/pkg/server/server.go around line 124:
`RequestSimulation` always returns `codes.Unimplemented`, so every simulation request fails and no `SimulationResponse` is ever produced. The previous implementation called `s.QuantumMirror.Simulate` and returned aggregate results; the cleanup that removed the phantom `QuantumMirror` field also deleted the working handler body. If this RPC is still wired to live callers (the gRPC handler and `ConnectLiteHandler`), return a real `SimulationResponse` or remove the RPC registration so clients get a consistent contract rather than a runtime error.
| return nil, status.Errorf(codes.Unimplemented, "engine: Profit Vortex deferred — re-architecture pending (ZTR cleanup)") | ||
| } | ||
|
|
||
| func (s *SovereignServer) GetTreasury(ctx context.Context, req interface{}) (interface{}, error) { |
There was a problem hiding this comment.
🟠 High server/server.go:468
GetTreasury returns a hard-coded balance of 0, so callers always receive zero regardless of the actual treasury state. A nonzero treasury is silently reported as empty, returning misleading financial data in a successful response. If the previous GlobalTreasury was removed because it was phantom, consider wiring this to a real ledger/treasury source or documenting explicitly that the endpoint is a stub until re-architecture.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @sidecar/sovereign-engine/pkg/server/server.go around line 468:
`GetTreasury` returns a hard-coded `balance` of `0`, so callers always receive zero regardless of the actual treasury state. A nonzero treasury is silently reported as empty, returning misleading financial data in a successful response. If the previous `GlobalTreasury` was removed because it was phantom, consider wiring this to a real ledger/treasury source or documenting explicitly that the endpoint is a stub until re-architecture.
Summary
Verification
Note
Migrate Go module path to
github.com/pai-list/PiWorkerand overhaul CI with pinned actions and Trivy scanninggithub.com/Moeabdelaziz007/PiWorker-OStogithub.com/pai-list/PiWorkeracross all Go source files, imports, and go.mod.actions/checkout,setup-node, andsetup-goto stable versions, adds pnpm as primary installer, replaces Trivy direct usage withaquasecurity/trivy-action@masterproducing SARIF output uploaded viacodeql-action/upload-sarif, and removes the separatee2e-realjob.|| true), includingnpm audit,typecheck,govulncheck, andsecretlint, and downgrades audit enforcement to critical-only.ZeroCostInferenceEngine(NVIDIA NIM / Gemini fallback),SuperteamJobEngine(bounty discovery and claiming),PiNodeComputeNetwork(node registration and tier-based reward splits), and a Telegram bot controller.RequestSimulationandEvaluateVortexgRPC handlers in server.go withUnimplementederrors and removesQuantumMirror/ProfitVortexfields fromSovereignServer.|| true, so regressions in audit, typecheck, and govulncheck will no longer block merges.Macroscope summarized fba073f.