Repository navigation
feat(citrineos): add a CitrineOS CSMS driver - #6
Conversation
`e2e` chained the two sweeps with `&&`, so a failing `run-all` skipped the authorize group entirely -- the three TC_023 scenarios that prove `driver provision` seeded any CSMS-side state at all, and therefore the ones worth most precisely when something else has already gone red. Run both, and exit non-zero if either failed. Observed in practice: a sweep whose 44-scenario pass failed still reported TC_023, which is what surfaced a CSMS mapping defect that would otherwise have been invisible that run.
CitrineOS (LF Energy / S44) is the second CSMS this harness drives, and the first one the 47 scenarios were not written against -- which is the point. An abstraction with a single implementation is CSMS-neutral by assertion; this turns that into a measurement. Operations go to the generated OCPP 1.6 message API (`POST /ocpp/1.6/<module>/<action>`); observations come from Postgres through `docker exec`, because the REST data endpoints expose none of what the scenarios assert on -- every `@AsDataEndpoint` was read to establish that. records.ts records why the bundled Hasura sidecar was not used instead. Both CitrineOS lines are supported, selected by CITRINE_VARIANT and defaulting to v2. v1.9.1 names the OCPP connection column `stationId` where v2 names it `ocppConnectionName`, and routes no 1.6 local auth list. The variant is declared rather than detected, so the scope table stays readable with no server and no credentials; `driver verify` then compares the declaration against the running schema and refuses to go further on a mismatch. Measured against the pinned images, two runs each, second from `down -v`: v2.0.0-beta1 38 PASS, 7 NOT APPLICABLE, 2 FAIL (of 47) v1.9.1 18 PASS, 13 NOT APPLICABLE, 16 FAIL Every group passes unmodified on v2 -- core, remote trigger, smart charging, local auth list, firmware -- and no scenario needed editing to accommodate the new CSMS. The seven NOT APPLICABLE are one missing capability: CitrineOS routes no OCPP 1.6 reservation endpoint. The OCA test plan gates those cases on a declared "R-0 Support for Reservations: Yes/No", so reporting them out of scope is what the reference intends. The two failures are kept red on purpose -- tck/scope.ts forbids demoting a row to hide a finding -- and both are named, with the source fact that causes them, in the driver's gap table. On v1.9.1 fourteen of the sixteen failures share one cause, upstream citrineos/citrineos#160: no Connectors row can be created for an ad-hoc 1.6 station, so nothing needing a transaction starts. Fixed on the v2 line, which is why v2 is the default. Refs #2
The idTag list is a property of the pinned simulator image, so each driver carries its own hard-coded copy -- they cannot share a constant when a driver may live in another repository entirely. That makes a second driver reintroduce exactly the drift this script exists to catch: it would report "add it to SteVe", SteVe would go green, and the other driver would keep failing TC_013/014/017/018 with an Authorize:Invalid that reads like a CSMS bug. Discover provisioners with a glob rather than naming them, so a third driver is covered the day it lands, and fail when the glob is empty -- an unchecked run that reports success is the failure mode being removed here, not a convenience.
"Reference driver" described a repository with one driver. With two, the label ranks them, and the ranking is wrong: each answers a question the other cannot. SteVe is where a capability the harness LOST shows up, because the scenarios were written against it and its scope table claims all 47. CitrineOS is where the core would be caught assuming something about SteVe, because it had no part in writing them. Renames `check:reference-driver` to `check:steve-driver`, alongside the existing `check:citrineos-driver`, and reframes the README, CONTRIBUTING and the SteVe driver header accordingly. Uses of "reference" that mean the OCA certification documents are deliberately left alone.
Adds `e2e-citrineos` beside the renamed `e2e-steve`, plus the offline `check-driver` step for the second driver -- the one whose scope table has NOT_APPLICABLE rows, so the only place where adding or renaming a scenario silently stops being covered. Both jobs run `bun run e2e`, so they cannot drift from each other or from what a human runs by hand. `continue-on-error` on the CitrineOS job is not a reliability hedge: the job is expected to be red on two named scenarios, both of which are findings kept red deliberately. Its value is the other 45 and the artifact it uploads. It is set per STEP on the sweeps as well as on the job, because a failing step ends a job even under a job-level continue-on-error -- which would skip the authorize sweep, the one containing the known failure. Refs #2
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 36009011 | Triggered | Generic Password | 864a0e4 | drivers/citrineos/compose.yaml | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
📝 WalkthroughWalkthroughThis change adds a CitrineOS OCPP 1.6 driver with API and database integrations, variant handling, provisioning, Compose environments, documentation, package exports, fixture validation, and parallel CI coverage. SteVe now uses its WebAPI for tag and transaction observations. ChangesCitrineOS driver and integration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Harness
participant csmsDriver
participant CitrineRecords
participant CitrineMessageApi
participant CitrineOS
Harness->>csmsDriver: Execute OCPP operation
csmsDriver->>CitrineRecords: Resolve records and transaction references
csmsDriver->>CitrineMessageApi: Send CitrineRequest
CitrineMessageApi->>CitrineOS: POST OCPP action
CitrineOS-->>CitrineMessageApi: Return confirmation
CitrineMessageApi-->>csmsDriver: Return receipt
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
|
@shiv3 fyi |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
drivers/citrineos/variant.ts (1)
122-129: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider binding these ids to the scope table keys.
V1_LOCAL_LIST_SCENARIOSholds free-form strings.v1Scope()indrivers/citrineos/scope.tsassignsna(NO_LOCAL_LIST)for each id without checking that the id exists inV2_SCOPE. A typo therefore creates a phantom scope row instead of a compile error. One option is to type the list asreadonly (keyof typeof V2_SCOPE)[]at the consumer, or to assert membership inv1Scope().🤖 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 `@drivers/citrineos/variant.ts` around lines 122 - 129, Bind V1_LOCAL_LIST_SCENARIOS to the valid scope keys exposed by V2_SCOPE so every scenario id is compile-time checked. Update the v1Scope() integration or the list declaration to use keyof typeof V2_SCOPE, preserving the existing scenario values and na(NO_LOCAL_LIST) behavior.drivers/citrineos/scope.ts (1)
242-244: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider returning a copy of
V2_SCOPE.
citrineosScope("v2")returns the module-levelV2_SCOPEobject itself.v1Scope()returns a fresh object. A caller that mutates the returned table therefore changes the shared v2 table for every later caller. Return{ ...V2_SCOPE }, or freeze the constant.🤖 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 `@drivers/citrineos/scope.ts` around lines 242 - 244, Update citrineosScope so the "v2" branch returns a shallow copy of V2_SCOPE rather than the shared module-level object, preserving the existing v1Scope() behavior and preventing caller mutations from affecting future results.drivers/citrineos/provision.ts (1)
195-215: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc blocks are attached to the wrong declaration. In two source files a doc block written for one declaration sits directly above the next declaration, so the generated declaration files inherit the same placement.
drivers/citrineos/provision.ts#L195-L215: move the "Read-only. Two queries rather than one per fixture" block from aboveverifySchemato aboveverify().drivers/citrineos/scope.ts#L196-L208: move the block describing the derived v1.9.1 table from aboveV1_KNOWNto abovev1Scope().types/drivers/citrineos/provision.d.ts#L46-L66: regenerate after theprovision.tsfix so the declaration matches.🤖 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 `@drivers/citrineos/provision.ts` around lines 195 - 215, The documentation blocks are attached to the wrong declarations. In drivers/citrineos/provision.ts:195-215, move the “Read-only. Two queries rather than one per fixture” block above verify(); in drivers/citrineos/scope.ts:196-208, move the derived v1.9.1 table block above v1Scope(); then regenerate types/drivers/citrineos/provision.d.ts:46-66 so its declaration documentation matches the corrected source.drivers/citrineos/records.ts (2)
303-312: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the row selection deterministic in
txScalar.The
LEFT JOIN "StopTransactions"can produce more than one row for one transaction.scalar()then returns the first line that psql emits, and psql applies no defined order withoutORDER BY.transactionStopTimestampandtransactionStopReasoncan therefore return different values across runs on the same data. Add an explicit order and a limit.♻️ Proposed change
LEFT JOIN "StopTransactions" st ON st."transactionDatabaseId" = t.id - WHERE t.id = ${sqlLiteral(tx)}::int AND t."tenantId" = ${this.tenant};`, + WHERE t.id = ${sqlLiteral(tx)}::int AND t."tenantId" = ${this.tenant} + ORDER BY st."timestamp" DESC NULLS LAST + LIMIT 1;`,🤖 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 `@drivers/citrineos/records.ts` around lines 303 - 312, Update txScalar to make joined-row selection deterministic by ordering the query with the appropriate StopTransactions field and limiting it to one row before scalar() reads the result. Preserve the existing transaction and tenant filters.
100-149: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a bounded wait on the
docker execprocess.
raw()awaitsproc.exitedwith no timeout. Ifdocker execorpsqlhangs, for example on a lock held by another session, the call never settles.waitForConditioncannot bound it either, because its poll awaits this same promise. A timeout that kills the process converts a hang into a message.♻️ Sketch
- const [stdout, stderr, exitCode] = await Promise.all([ + const timer = setTimeout(() => proc.kill(), DB_TIMEOUT_MS); + const [stdout, stderr, exitCode] = await Promise.all([ new Response(proc.stdout).text(), new Response(proc.stderr).text(), proc.exited, - ]); + ]).finally(() => clearTimeout(timer));🤖 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 `@drivers/citrineos/records.ts` around lines 100 - 149, Add a bounded timeout to the process wait in raw, ensuring a hung docker exec or psql process is terminated and raw rejects with a clear timeout error. Update the Promise.all flow around proc.exited and use the existing process handle proc to kill the child when the deadline expires, while preserving normal stdout, stderr, and exit-code handling.
🤖 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 `@drivers/citrineos/api-client.ts`:
- Around line 62-80: Move the response body read in send into the existing
try/catch that wraps fetch, so timeout or other body-stream failures are
converted to the same contextual error format. Preserve the current URL,
`citrineos:` prefix, and non-Error handling used by the catch block.
- Around line 39-41: Update the describe function to always return a meaningful
string when JSON.stringify(value) yields undefined, preserving a diagnostic
representation for missing payloads. Ensure callers such as the confirmation
error construction retain that fallback in the final failure message.
In `@drivers/citrineos/compose.yaml`:
- Around line 109-112: Keep the intentional local Postgres credentials in
drivers/citrineos/compose.yaml:109-112 and add a root .gitguardian.yaml
exception scoped only to this exact occurrence, without ignoring the file or
Generic Password detector; ensure the external scanner loads the repository
configuration. In drivers/citrineos/README.md:57-62, replace “No credentials
anywhere” with “No HTTP API credentials are required.” Preserve the existing
citrine and guest defaults rather than making credentials mandatory.
In `@package.json`:
- Line 115: Update the package.json e2e script to replace the POSIX arithmetic
expansion in its final status calculation with shell-independent logic supported
by Bun Shell on Windows, while preserving execution of both test commands and
returning a failing exit status if either command fails.
In `@tools/extract-fixture-tags.sh`:
- Line 90: Update the driver_tags assignment in the provisioner-checking loop to
tolerate grep returning no matches under set -euo pipefail, producing an empty
tag set instead of aborting. Ensure execution continues to the existing missing
branch so a FAIL line identifies the affected driver and subsequent provisioners
are still processed.
---
Nitpick comments:
In `@drivers/citrineos/provision.ts`:
- Around line 195-215: The documentation blocks are attached to the wrong
declarations. In drivers/citrineos/provision.ts:195-215, move the “Read-only.
Two queries rather than one per fixture” block above verify(); in
drivers/citrineos/scope.ts:196-208, move the derived v1.9.1 table block above
v1Scope(); then regenerate types/drivers/citrineos/provision.d.ts:46-66 so its
declaration documentation matches the corrected source.
In `@drivers/citrineos/records.ts`:
- Around line 303-312: Update txScalar to make joined-row selection
deterministic by ordering the query with the appropriate StopTransactions field
and limiting it to one row before scalar() reads the result. Preserve the
existing transaction and tenant filters.
- Around line 100-149: Add a bounded timeout to the process wait in raw,
ensuring a hung docker exec or psql process is terminated and raw rejects with a
clear timeout error. Update the Promise.all flow around proc.exited and use the
existing process handle proc to kill the child when the deadline expires, while
preserving normal stdout, stderr, and exit-code handling.
In `@drivers/citrineos/scope.ts`:
- Around line 242-244: Update citrineosScope so the "v2" branch returns a
shallow copy of V2_SCOPE rather than the shared module-level object, preserving
the existing v1Scope() behavior and preventing caller mutations from affecting
future results.
In `@drivers/citrineos/variant.ts`:
- Around line 122-129: Bind V1_LOCAL_LIST_SCENARIOS to the valid scope keys
exposed by V2_SCOPE so every scenario id is compile-time checked. Update the
v1Scope() integration or the list declaration to use keyof typeof V2_SCOPE,
preserving the existing scenario values and na(NO_LOCAL_LIST) behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e947c98-1613-46c2-a00f-31b8d75d3c1c
📒 Files selected for processing (30)
.github/workflows/ci.ymlCONTRIBUTING.mdREADME.mdVENDOR.mddrivers/citrineos/README.mddrivers/citrineos/api-client.tsdrivers/citrineos/compose.v1.yamldrivers/citrineos/compose.yamldrivers/citrineos/config.tsdrivers/citrineos/index.tsdrivers/citrineos/profiles.tsdrivers/citrineos/provision.tsdrivers/citrineos/records.tsdrivers/citrineos/requests.tsdrivers/citrineos/scope.tsdrivers/citrineos/variant.tsdrivers/steve/index.tspackage.jsontools/extract-fixture-tags.shtsconfig.build.jsontypes/drivers/citrineos/api-client.d.tstypes/drivers/citrineos/config.d.tstypes/drivers/citrineos/index.d.tstypes/drivers/citrineos/profiles.d.tstypes/drivers/citrineos/provision.d.tstypes/drivers/citrineos/records.d.tstypes/drivers/citrineos/requests.d.tstypes/drivers/citrineos/scope.d.tstypes/drivers/citrineos/variant.d.tstypes/drivers/steve/index.d.ts
| environment: | ||
| POSTGRES_DB: citrine | ||
| POSTGRES_USER: citrine | ||
| POSTGRES_PASSWORD: citrine |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target files ---'
sed -n '90,125p' drivers/citrineos/compose.yaml
sed -n '45,75p' drivers/citrineos/README.md
printf '%s\n' '--- scanner and GitGuardian configuration references ---'
rg -n -i --hidden \
-g '!node_modules' -g '!dist' -g '!build' \
'gitguardian|ggshield|secret\.ignored_matches|ignore(d)?_matches|generic password|CITRINE_DB_PASS|POSTGRES_PASSWORD' \
.
printf '%s\n' '--- tracked configuration files ---'
git ls-files | rg -i '(^|/)(\.github|.*gitguardian|.*ggshield|.*secret|.*scan|.*compose|.*readme)'Repository: juherr/open-ocpp-tck
Length of output: 3873
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- CitrineOS configuration and startup paths ---'
sed -n '1,210p' drivers/citrineos/config.ts
sed -n '130,190p' drivers/citrineos/index.ts
sed -n '140,175p' drivers/citrineos/README.md
printf '%s\n' '--- Compose credential and environment usage ---'
rg -n -C 4 \
'POSTGRES_(DB|USER|PASSWORD)|PGPASSWORD|CITRINE_DB_PASS|AMQP|RABBIT|environment:|env_file|\.env' \
drivers/citrineos drivers/steve
printf '%s\n' '--- CI workflow ---'
sed -n '1,260p' .github/workflows/ci.yml
printf '%s\n' '--- repository-level ignore/config files ---'
git ls-files -a | rg '(^|/)(\.gitignore|\.github/.*|.*ggshield.*|.*gitguardian.*|.*secret.*|.*pre-commit.*)$' | while IFS= read -r f; do
printf '\n--- %s ---\n' "$f"
sed -n '1,240p' "$f"
doneRepository: juherr/open-ocpp-tck
Length of output: 33295
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- CitrineOS Compose services and exposure ---'
sed -n '1,145p' drivers/citrineos/compose.yaml
sed -n '1,120p' drivers/citrineos/compose.v1.yaml
printf '%s\n' '--- package scripts and all workflow/config files ---'
sed -n '1,220p' package.json
find .github -maxdepth 3 -type f -print 2>/dev/null | sort | while IFS= read -r f; do
printf '\n--- %s ---\n' "$f"
sed -n '1,220p' "$f"
done
printf '%s\n' '--- tracked and untracked scanner-related files ---'
git ls-files | rg -i 'gitguardian|ggshield|secret|scanner|security|workflow|gitignore' || true
find . -maxdepth 3 -type f \( -iname '*gitguardian*' -o -iname '*ggshield*' -o -iname '*secret*' \) -print 2>/dev/null | sort || true
printf '%s\n' '--- read-only credential-shape check ---'
python3 - <<'PY'
from pathlib import Path
for path in [Path("drivers/citrineos/compose.yaml"),
Path("drivers/citrineos/README.md"),
Path("drivers/citrineos/config.ts"),
Path("drivers/citrineos/index.ts")]:
text = path.read_text()
print(f"{path}:")
for i, line in enumerate(text.splitlines(), 1):
if any(term in line for term in (
"POSTGRES_PASSWORD", "RABBITMQ_DEFAULT_PASS", "CITRINE_DB_PASS",
"No credentials", "No HTTP API credentials", "default citrine",
"secret.ignored_matches", "ggshield", "gitguardian",
)):
print(f" {i}: {line}")
PYRepository: juherr/open-ocpp-tck
Length of output: 23064
🌐 Web query:
GitGuardian ggshield configuration secret.ignored_matches exact syntax and CI configuration file
💡 Result:
In ggshield, the secret.ignored_matches setting is used to exclude specific secrets from being reported as incidents [1][2]. Configuration Syntax The configuration is defined within a.gitguardian.yaml file using the following structure [1][3]: secret: ignored_matches: - name: "Optional descriptive name" match: "SHA256_of_occurrence_or_the_secret_itself" You can find the SHA256 value of a secret in the ggshield scan output when an incident is detected [1]. Alternatively, you can use the command line tool to automatically add secrets to this configuration: ggshield secret ignore --name "my-secret-name" [4] If you want to ignore all secrets found during your most recent scan, you can run: ggshield secret ignore --last-found [4] CI Configuration When running ggshield in a CI environment, the same.gitguardian.yaml file is used if it is present in your repository [3]. ggshield automatically looks for this file in the root directory of the repository being scanned [3]. Best practices for CI: 1. Ensure your.gitguardian.yaml file is committed to your repository so that it is available to the CI runner [3]. 2. If you do not want to manage ignored secrets in a static file, you can also use command-line arguments during your CI scan, such as --ignore-known-secrets, which instructs ggshield to ignore secrets that are already marked as known on your GitGuardian dashboard [1][5]. Note on syntax versions: Ensure your configuration file includes version: 2 at the top to avoid using the deprecated v1 format [1][6]. While some older documentation may use hyphenated keys (e.g., ignored-matches), the current recommended syntax uses underscores (ignored_matches) as shown in the official documentation [1][3].
Citations:
- 1: https://docs.gitguardian.com/ggshield-docs/configuration
- 2: https://github.com/GitGuardian/ggshield/blob/851fcc3357f254d4529595107b8825847118d303/.gitguardian.example.yml
- 3: https://docs.gitguardian.com/endpoint-protection/configuration-reference
- 4: https://docs.gitguardian.com/ggshield-docs/reference/secret/ignore
- 5: https://docs.gitguardian.com/ggshield-docs/reference/secret/scan/ci
- 6: https://pypi.org/project/ggshield/1.13.5/
Scope the GitGuardian exception and correct the credential documentation.
The stack intentionally uses local Postgres (citrine) and RabbitMQ (guest) credentials. CITRINE_DB_PASS also defaults to citrine, and CI does not provide an override. Do not make the variable mandatory without updating the Compose file, driver defaults, README, and CI.
If GitGuardian blocks this fixture, add a root .gitguardian.yaml entry for this exact occurrence. Do not ignore the file or the Generic Password detector. The checked-in CI workflow has no GitGuardian step, so ensure the external scanner loads this repository configuration.
Replace “No credentials anywhere” with “No HTTP API credentials are required.”
🧰 Tools
🪛 GitHub Check: GitGuardian Security Checks
[error] 112-112: GitGuardian detected a hardcoded generic password secret. Revoke and rotate the secret, remove it from the file and git history if possible, and store it securely.
📍 Affects 2 files
drivers/citrineos/compose.yaml#L109-L112(this comment)drivers/citrineos/README.md#L57-L62
🤖 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 `@drivers/citrineos/compose.yaml` around lines 109 - 112, Keep the intentional
local Postgres credentials in drivers/citrineos/compose.yaml:109-112 and add a
root .gitguardian.yaml exception scoped only to this exact occurrence, without
ignoring the file or Generic Password detector; ensure the external scanner
loads the repository configuration. In drivers/citrineos/README.md:57-62,
replace “No credentials anywhere” with “No HTTP API credentials are required.”
Preserve the existing citrine and guest defaults rather than making credentials
mandatory.
Source: Pipeline failures
| "e2e": "bun bin/ocpp-tck.ts run-all --parallel --retry-failed-isolated && bun bin/ocpp-tck.ts run-all --group authorize", | ||
| "check:steve-driver": "CSMS_DRIVER=./drivers/steve/index.ts bun bin/ocpp-tck.ts check-driver", | ||
| "check:citrineos-driver": "CSMS_DRIVER=./drivers/citrineos/index.ts bun bin/ocpp-tck.ts check-driver", | ||
| "e2e": "bun bin/ocpp-tck.ts run-all --parallel --retry-failed-isolated; all=$?; bun bin/ocpp-tck.ts run-all --group authorize; auth=$?; exit $(( all || auth ))", |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Bun shell used by bun run package.json scripts support POSIX arithmetic expansion $(( )) and $? ?
💡 Result:
No, the Bun Shell does not support POSIX arithmetic expansion $(( )) [1]. Attempts to use this syntax will typically not function as expected, and it is recommended to use tools like awk within your commands to perform arithmetic operations [1]. Regarding the special shell variable $? (which typically holds the exit status of the last executed command), Bun Shell's support has evolved. While earlier versions or specific contexts might have treated it as literal text [2], recent updates have explicitly worked to implement support for $?, along with other variables like $$ and $# [2]. It is important to note that when running package.json scripts: 1. On Linux and macOS, Bun typically defaults to system shells (bash, sh, or zsh) [3][4]. In these environments, your scripts will inherit the POSIX support of the shell being used. 2. On Windows, Bun uses its own embedded Bun Shell [3][4]. Because the Bun Shell is an experimental, custom implementation, it does not support the full POSIX specification, including features like arithmetic expansion [5][1]. If you rely on specific POSIX shell features for cross-platform scripts, you should verify if they are supported by the Bun Shell or ensure your environment uses a standard POSIX-compliant shell [3][4].
Citations:
- 1: https://www.skill-gallery.jp/en/skills/shanepadgett/bun-shell-commands
- 2: shell: implement
$?,$$,$#, and${NAME}; reject unsupported expansions oven-sh/bun#33274 - 3: https://bun.sh/docs/runtime
- 4: https://github.com/oven-sh/bun/blob/1498d7b7/docs/runtime/index.mdx
- 5: https://bun.sh/blog/the-bun-shell
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- package manager and script context ---'
sed -n '1,140p' package.json
printf '%s\n' '--- workflow invocations ---'
rg -n -C 4 'bun run e2e|npm run e2e|yarn e2e|pnpm e2e|run-all|bun-version|oven-sh/setup-bun' .github/workflows package.json 2>/dev/null || true
printf '%s\n' '--- repository shell-related configuration ---'
rg -n -i -C 2 'bun([ -])run|script-shell|BUN|shell' package.json bunfig.toml .bunfig.toml .github 2>/dev/null || trueRepository: juherr/open-ocpp-tck
Length of output: 20247
🌐 Web query:
site:bun.sh/docs/runtime bun run package.json scripts shell Bun Shell POSIX arithmetic expansion
💡 Result:
Bun Shell does not natively support POSIX arithmetic expansion (e.g.,
Citations:
- 1: https://bun.sh/docs/runtime/shell
- 2: https://bun.sh/docs/runtime
- 3: https://bun.sh/docs/runtime/bunfig
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- available Bun runtime ---'
if command -v bun >/dev/null 2>&1; then
bun --version
bun run --help 2>&1 | sed -n '1,180p'
else
echo 'bun: unavailable'
fi
printf '%s\n' '--- local documentation or configuration references ---'
rg -n -i -C 3 'shell\s*=|Bun Shell|bun shell|arithmetic expansion|bun run.*shell' \
. --glob '!package-lock.json' --glob '!bun.lockb' --glob '!node_modules/**' 2>/dev/null | sed -n '1,240p'Repository: juherr/open-ocpp-tck
Length of output: 261
🌐 Web query:
site:bun.sh/docs/runtime/bunfig shell = "bun" shell = "system" bun run default shell version 1.3.14
💡 Result:
In Bun, the shell configuration determines how package.json scripts are executed by bun run or the bun command [1]. The shell setting can be configured in the bunfig.toml file within the [run] section [1]. The available options are: - system: This uses the platform's default system shell [1]. This is the default behavior on all platforms except Windows [1]. - bun: This uses Bun's built-in shell [1]. This is the default behavior on Windows [1]. To explicitly set the shell to the system shell, use the following configuration [1]: [run] shell = "system" To explicitly set the shell to use Bun's built-in shell, use the following configuration [1]: [run] shell = "bun" Regarding the version mentioned, note that while current documentation covers these configuration options, specific details regarding a version 1.3.14 were not confirmed in the available results [1].
Citations:
🌐 Web query:
site:github.com/oven-sh/bun 1.3.14 bun run shell arithmetic expansion $(( )) $?
💡 Result:
Bun's shell execution behavior depends on whether you are using the built-in "Bun Shell" (common on Windows or when explicitly invoked via the $ tag in JavaScript) or a system shell (often used on Linux/macOS when executing package.json scripts) [1][2]. Regarding your specific query: 1. Arithmetic Expansion $ template literal tag in JavaScript, you are using Bun's own built-in shell implementation, which is designed for cross-platform compatibility [5][6][1]. It includes many common features like command substitution $(...), glob expansion, and piping, but it does not mirror the full feature set of Bash, including advanced arithmetic expansion [5][6][3]. - System Shells: When you run package.json scripts on Linux or macOS, Bun typically delegates execution to a system shell (e.g., bash, sh, or zsh) [1][2]. In these cases, the behavior of
Citations:
- 1: https://github.com/oven-sh/bun/blob/1498d7b7/docs/runtime/index.mdx
- 2: https://github.com/oven-sh/bun/blob/bb66bba1bf6d57983e7efef7ab706bce12137028/src/cli/run_command.zig
- 3: feat: Bun shell oven-sh/bun#7748
- 4: shell: implement
$?,$$,$#, and${NAME}; reject unsupported expansions oven-sh/bun#33274 - 5: https://github.com/oven-sh/bun/blob/b433beb016470b87850f3c018974de5f2e355d52/docs/runtime/shell.md
- 6: https://github.com/oven-sh/bun/blob/a0e221e0/docs/runtime/shell.mdx
Make e2e portable to Windows.
bun run uses the system shell by default on Linux and macOS, so both Ubuntu CI jobs support this arithmetic expansion. Windows uses Bun Shell, which does not support $((...)). Manual Windows runs can therefore fail. Replace the arithmetic expansion with shell-independent status logic or a Bun/TypeScript helper.
🤖 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 `@package.json` at line 115, Update the package.json e2e script to replace the
POSIX arithmetic expansion in its final status calculation with
shell-independent logic supported by Bun Shell on Windows, while preserving
execution of both test commands and returning a failing exit status if either
command fails.
AbortSignal.timeout covers the response body, not just the headers, so a CSMS that answers its headers and then stalls threw a bare TimeoutError naming no URL. Read the body inside the same try, and give describe() a fallback for the payload-less confirmation, where JSON.stringify returns undefined rather than a string and emptied the failure message.
Nothing constrains StopTransactions to one row per transaction -- a charge point that retries StopTransaction gets a second one -- and scalar() then read whichever row the planner emitted first. Order by the stop instant and take the latest, which is the report the station stands by. Verified on the pinned postgis image: two stop rows resolve to the later one, and a transaction with none still falls back to Transactions.
V2_SCOPE now satisfies ScopeTable instead of being annotated with it, so its keys stay literal and the id list variant.ts owns can be declared as keyof typeof V2_SCOPE. A typo there was previously caught only by check-driver under CITRINE_VARIANT=v1, which CI does not run; it is now a build error.
The paragraph is about LocalBypassAuthProvider, but "no credentials anywhere" also reads as a claim about the stack, which does set a Postgres password in compose.yaml and forwards it as PGPASSWORD.
grep exits 1 on no match, and under set -euo pipefail a command substitution that fails aborts the assignment -- so a driver provisioning no CERT tag at all killed the whole check silently, before the FAIL line meant to name it. An empty set is the honest answer, and it falls straight into the missing branch.
Both provisioners wrote a hardcoded 2020 instant into the expired-tag fixture, so the row claimed to have expired years before any activity recorded against it. Date it from the CSMS's own clock at provisioning time instead, one minute back so the strictly-ordered "is this expired" comparison is unambiguous whatever the clock granularity on either side. It is a backdate, not a wait: nothing sleeps. That is what steve-community/steve#2100 settled on for the endpoint it proposes -- expire with now() rather than accept an arbitrary past date, because a fabricated date is bad data hygiene. Adopting that endpoint later becomes a call swap rather than a change of meaning. The offset and the reasoning live in tck/time.ts as a number of minutes, never an instant: each driver subtracts it server-side, so no timestamp this process computed reaches a fixture. The fixed date was never load-bearing -- both verifiers compare the relation, not the literal, and nothing else in the repository mentions it.
The driver answered every observation out of MariaDB through docker
exec, on the stated grounds that SteVe's WebAPI "does not expose
stop_reason". Measured against the pinned image, that was wrong:
GET /api/v1/transactions serves stopReason, stopTimestamp and ocppIdTag
on the Transaction DTO, filters by chargeBoxId, ocppIdTag, transactionPk
and type=ACTIVE with no default date window, and PATCH
/transactions/{pk}/stop closes a transaction without putting anything on
the wire -- which is exactly what the stale-transaction hook needs.
So transactions, fixture verification and tag teardown now go through
the API, and the database is left with precisely what SteVe has no
endpoint for, each naming its ticket:
expiry write steve-community/steve#2100
WebAPI credential bootstrap steve-community/steve#2075, #2059
reservations steve-community/steve#2074
charging profiles steve-community/steve#2069
That is the property to preserve: every remaining database access in
this driver is one of those four, and nothing else. Asserting through
the API is also the stronger claim -- an integrator can reproduce it,
where a claim about SteVe's private tables cannot be.
A shared client (api-client.ts) now serves both the provisioner and the
records reader; appContainer moves to SteveConfig, beside dbContainer,
since a container name is deployment rather than transport. verify()
trades two properties for this, deliberately: it no longer works on an
environment where API access was never enabled, and it compares the
expiry against this process's clock -- which the fixture's one-minute
backdate covers.
Refs #8: 44 of the 51 record calls a sweep makes no longer spawn a
process. The issue stays open -- the SQL transport is unchanged, and the
CitrineOS driver has no API to move to.
TC_044 asked for `retrieveDate: inSeconds(90)`, and a driver may only round that value UP -- the contract says so, because a CSMS whose form has no seconds field must still receive a strictly future instant. SteVe's UpdateFirmware form is minute-resolution, so the charge point waited 90-150s against a 115s window: the scenario passed or failed on where the wall clock happened to sit within the minute, and since the three run back to back at a ~120s period they drew the same phase together -- one unlucky sweep failed all three. Asking for 15s instead puts the worst case at 75s, leaving the status train and a tail buffer inside the same window. The offset was 90s for a reason that no longer holds: it dated from when the rounding truncated DOWN and a small offset could land in the already-past minute. Reproduced and fixed under measurement rather than inferred: starting the scenario at a wall-clock second that makes the rounding worst-case, tc044-1 failed on the old offset and passed on the new one at the very same phase. The whole firmware group then passed on both drivers. Fixes #4
refactor(drivers): prefer the CSMS API over its database
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 (1)
drivers/steve/provision.ts (1)
331-351: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDrifted-tag deletion has no reference guard, unlike teardown.
teardowndocuments thattransaction_start.id_tagandreservation.id_tagcascade on delete, and it callsidTagsReferencedbefore every delete for that reason.provisionTagsdeletes a drifted tag with no such guard on Line 337 and Line 344. If a re-provision runs on an environment that already has transactions for a tag whoseexpiryDateorblockedstate drifted, the delete removes that history silently.Reuse the same guard, or state in the comment why a drifted tag can never be referenced.
🤖 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 `@drivers/steve/provision.ts` around lines 331 - 351, Add the existing idTagsReferenced guard used by teardown before both drifted-tag deletions in provisionTags, covering regular VALID_TAGS rows and the BLOCKED_TAG row. Preserve the current delete-and-recreate flow for unreferenced tags, and handle referenced tags according to the established teardown behavior rather than deleting their history.
🧹 Nitpick comments (1)
drivers/steve/api-client.ts (1)
59-61: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
btoathrows on non-ASCII credentials.
btoaaccepts only Latin-1 code points. If an operator setsSTEVE_API_PASSto a value with a non-Latin-1 character, the constructor throwsInvalidCharacterErrorbefore any request runs, and the message does not name the credential. Encode the bytes first.♻️ Proposed encoding fix
constructor(private readonly cfg: SteveApiConfig) { - this.authorization = `Basic ${btoa(`${cfg.username}:${cfg.password}`)}`; + const raw = new TextEncoder().encode(`${cfg.username}:${cfg.password}`); + this.authorization = `Basic ${btoa(String.fromCharCode(...raw))}`; }🤖 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 `@drivers/steve/api-client.ts` around lines 59 - 61, Update the Steve API client constructor’s authorization initialization to encode the UTF-8 bytes of the username and password before Base64 conversion, so non-ASCII credentials are supported; preserve the existing Basic authorization format and ensure encoding failures identify the affected credential.
🤖 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 `@drivers/steve/provision.ts`:
- Around line 331-351: Add the existing idTagsReferenced guard used by teardown
before both drifted-tag deletions in provisionTags, covering regular VALID_TAGS
rows and the BLOCKED_TAG row. Preserve the current delete-and-recreate flow for
unreferenced tags, and handle referenced tags according to the established
teardown behavior rather than deleting their history.
---
Nitpick comments:
In `@drivers/steve/api-client.ts`:
- Around line 59-61: Update the Steve API client constructor’s authorization
initialization to encode the UTF-8 bytes of the username and password before
Base64 conversion, so non-ASCII credentials are supported; preserve the existing
Basic authorization format and ensure encoding failures identify the affected
credential.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e2d0cf0-b38b-45c1-b272-a5938dade863
📒 Files selected for processing (17)
VENDOR.mddrivers/citrineos/provision.tsdrivers/steve/api-client.tsdrivers/steve/index.tsdrivers/steve/provision.tsdrivers/steve/records.tsdrivers/steve/ui-client.tspatches/tck/specs/firmware.ts.patchtck/specs/firmware.tstck/time.tstypes/drivers/citrineos/provision.d.tstypes/drivers/steve/api-client.d.tstypes/drivers/steve/index.d.tstypes/drivers/steve/provision.d.tstypes/drivers/steve/records.d.tstypes/drivers/steve/ui-client.d.tstypes/tck/time.d.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- types/drivers/citrineos/provision.d.ts
- drivers/citrineos/provision.ts
- types/drivers/steve/index.d.ts
"key":[], so a conformant CSMS that omits the member is failed
#31
1 participant
Adds a CitrineOS driver, which is the first time these 47 scenarios have been
driven against a CSMS that had no part in writing them.
Advances #2 — the CitrineOS item. The other candidates on that checklist are
untouched, so this does not close it.
Result
Measured against the pinned images, two runs each, the second from
down -v:v2.0.0-beta1(default)v1.9.1No scenario needed editing to accommodate the new CSMS, and on v2 every
group passes unmodified — core, remote trigger, smart charging, local auth
list, firmware. That is the claim the second driver exists to support.
What the two lines cost
CITRINE_VARIANTselects the line and defaults tov2. v1.9.1 names the OCPPconnection column
stationIdwhere v2 names itocppConnectionName, androutes no 1.6 local auth list.
The variant is declared, not detected, because the scope table and the
capability set must be readable with no server and no credentials — that is
what keeps
check-driverand the pre-flight offline.driver verifythencompares the declaration against the running schema and refuses to go further
on a mismatch. The trap that makes detection tempting and wrong is recorded in
variant.ts:stationIdexists onTransactionsin both lines, as avarcharholding the OCPP name on v1.9.1 and anintegerforeign key on v2.Findings, and why two rows stay red
tck/scope.tsforbids demoting a row toNOT_APPLICABLEto make a redscenario go away, so both failures are visible rather than hidden:
{"idTagInfo":{"status":"Invalid"}}whereOCPP 1.6 requires
Blocked; 3 runs of 3, and identical on both lines, so itis a property of the
Authorizehandler rather than of a release. OCATC_023_3_CSMSgives the prerequisite as "an idTag in memory with status'Blocked'" — exactly what
driver provisionwrites — and marks it mandatory.and the CitrineOS data point is added there.
The seven
NOT APPLICABLEare one missing capability: no OCPP 1.6 reservationendpoint is routed. The OCA test plan gates those cases on a declared
R-0 Support for Reservations: Yes/No, so reporting them out of scope is whatthe reference intends rather than a convenience.
On v1.9.1, fourteen of the sixteen failures share one cause — upstream
citrineos/citrineos#160, noConnectorsrow for an ad-hoc 1.6 station, sonothing needing a transaction starts. Fixed on the v2 line, which is why v2 is
the default. Nothing was reported upstream for v1.9.1: it is already fixed
where it matters.
drivers/citrineos/README.mdcarries the full gap table, one source fact perrow.
Also in here
fix(e2e)—e2echained the sweeps with&&, so a failingrun-allskipped the authorize group: the three scenarios that prove provisioning
seeded anything. This bit in practice during the v1.9.1 run.
test(tools)—extract-fixture-tags.shchecked one driver by name; asecond driver reintroduced exactly the drift it exists to catch. Now globs
drivers/*/provision.tsand fails on an empty glob.refactor(drivers)— "reference driver" ranked two drivers that answerdifferent questions. Renames
check:reference-drivertocheck:steve-driverand reframes the docs. Uses of "reference" meaning the OCA documents are
left alone.
Verification
typecheck,types-current,vendor-integrity,generic-core,spec-invariants,shellcheck, andcheck-driverfor both drivers and bothvariants. Each commit type-checks on its own. Live runs against both pinned
images, with the second taken from a clean
down -v.Summary by CodeRabbit
New Features
Documentation
Tests