nvt M1-kjerne: nvt-bridge + agents/nvt-fat-developer bak filkontrakten - #110
nvt M1-kjerne: nvt-bridge + agents/nvt-fat-developer bak filkontrakten#110olebhansen-agent wants to merge 2 commits into
Conversation
…1-kjerne) Miljøuavhengig del av M1: ny utførende agent bak uendret filkontrakt, med en deterministisk bro (ingen LLM) som mapper topic til levende nvt-instans. apps/nvt-bridge/ (Node/TS, samme stack og testoppsett som integrations/): - dedupe som resten av pipelinen: id uten linje i results.jsonl - topic = payload.origin.event_id uten delta-suffiks (-dN), ellers eventets id - topic -> instans i state/topics.json; instansnavnet er deterministisk avledet og lengdebegrenset, så tapt state gjenfinner samme workspace - serielt per topic (én levende sesjon, én arbeidskopi), parallelt på tvers - TTL-basert agent-down som beholder workspacet - fallback: status:"error" med forklaring når signal done kommer uten resultatlinje, ved timeout, eller ved intern feil. Aldri en fabrikert suksess — status:"ok" kan kun komme fra agenten selv All nvt-interaksjon bak NvtDriver med fake for tester. Ekte adapter (src/nvt/docker.ts) er tynn og merket «kalibreres mot M0-funn» (#96): den er det eneste stedet antakelser om make-mål, containernavn og agentdctl-format bor. agents/nvt-fat-developer/: triggers-kontrakt, README, .env.example og instruks-malen for instansens AGENTS.local.md — kodeagent-protokollen ordrett der den kan, men uten innboks-polling og med agentdctl signal done etter resultatlinja. 62 tester dekker dedupe, topic-avledning, serialisering per topic og fallback-linja. integrations/src/ er urørt; ruta i AGENT_ROUTES er driftskonfig som tas ved utrulling. Refs #97 Co-Authored-By: Claude <noreply@anthropic.com>
Funn fra reviewer-subagent på diffen (ferske øyne). Alle fikser har regresjonstest; 62 -> 77 tester. - Blocker: bridge-loggen ble skrevet utenfor try/catch, så en feilende logg-skriving (f.eks. logs/ som fil -> ENOTDIR) etterlot eventet UTEN resultatlinje — dvs. evig ubehandlet, og polleren dispatcher i ring. Nå er loggingen best effort og innenfor try-en, og fallback-linja skrives før logging/opprydding i catch-en. - Major: bridgeLogRelPath brukte rå event-id. En id med `/` eller `..` ga en `log`-verdi integrations avviser (utenfor triggers/) og fikk loggen skrevet utenfor katalogen. Bruker nå safeId(), som fantes men var ubrukt. - Major: appendResult antok at fila slutter med linjeskift. Agenten skriver linja frihånds; glemte den linjeskiftet, limte vår append seg på dens linje og gjorde begge uleselige — agentens suksess tapt, eventet re-dispatchet. Reparerer nå manglende linjeskift først. - Major: serialisering per topic overlevde ikke omstart. Injiserte events markeres nå i state (in_flight_event_id) før prompten sendes; etter omstart promptes de ikke på nytt (en andre prompt inn i en levende sesjon er verre enn ærlig «ukjent utfall»). SIGTERM venter nå på events under arbeid, og poll-sleepen er abort-aware. - Major: prompt-injeksjon. Fast avgrenser lot oppgaveteksten «lukke» blokka og utgi seg for broen; siden integrations matcher kun på id, kunne en lurt agent postet falsk suksess i en ANNEN tråd. Avgrenserne har nå en nonce, og kontrakten med den eneste gyldige id-en gjentas ETTER blokka. - Minor: /triggers var hardkodet i prompt.ts stikk i strid med at alle nvt-antakelser skal ligge i docker.ts — nå konfigurerbart (NVT_INSTANCE_TRIGGERS_PATH). - Nits: .catch() på runTopic, grace-polling 500ms -> 1000ms (hvert forsøk leser hele results.jsonl), tomme event-felter rendres ikke som "undefined". Testhull lukket: nådefristen ble aldri utøvd (resultGraceMs var 0 overalt), og maks-parallell var utestet på bridge-nivå. Refs #97 Co-Authored-By: Claude <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Review av denne PR-en (reviewer-subagent, ferske øyne)Kjørt som prosessen krever, før rapportering. Reviewer fikk diffen uten Konklusjon: 1 blocker + 4 majors, alle reelle og alle reprodusert. Alle er Funn og hva som ble gjort
Verifisert OK (med bevis)
Status etter fiksEnde-til-ende tørrkjørt mot et ekte delegerings-event: topic avledet riktig, Merk at grønne tester her ikke er E2E-bevis: adapteren mot ekte |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
apps/nvt-bridge/src/topic.ts (1)
62-73: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
instanceNameForcan exceedmaxLengthwhenprefixalone leaves no room.
roomis clamped to≥0(Line 70), but the finalprefix-hash(slug dropped) is never re-checked againstmaxLength. E.g.prefix.length = 25,maxLength = 20,hash.length = 8→fixed = 35 > 20→room = 0,slug = "", but the returned name is still25 + 1 + 8 = 34chars — well over the configured cap. Since this cap exists specifically to satisfy the DNS-label limit that compose project names and code-server hostnames rely on (per the docstring), a silent overflow here defeats the whole point of the function.🛡️ Proposed fix — fail fast instead of silently overflowing
const hash = createHash("sha256").update(topic).digest("hex").slice(0, 8); const prefix = slugify(opts.prefix); // Fast del: <prefix>- ... -<hash8> const fixed = (prefix === "" ? 0 : prefix.length + 1) + 1 + hash.length; + if (fixed > opts.maxLength) { + throw new Error( + `instanceNameFor: prefix "${opts.prefix}" leaves no room for a slug within maxLength=${opts.maxLength}`, + ); + } const room = Math.max(0, opts.maxLength - fixed);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/nvt-bridge/src/topic.ts` around lines 62 - 73, Update instanceNameFor to validate that the prefix, separator, and hash can fit within opts.maxLength before constructing the result; when the fixed prefix-hash portion exceeds the limit, fail fast with a clear error instead of returning an overlong name. Preserve the existing slug truncation behavior when sufficient room remains.apps/nvt-bridge/src/nvt/docker.ts (1)
115-169: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
waitForDone's streaming/timeout/abort logic has zero test coverage because it isn't injectable.Every other method in
DockerNvtDriver(ensureInstance,sendPrompt,stopInstance,isRunning,make) is testable because it goes through the injectablethis.exec/ExecFnseam.waitForDoneinstead calls the importedspawndirectly, so its partial-line buffering, timeout, and abort-signal wiring — the most stateful logic in the file — has no unit test today.
apps/nvt-bridge/src/nvt/docker.ts#L115-L169: extract thespawncall behind an injectable function (mirroringExecFn, e.g. aSpawnFnpassed viaDockerNvtDriverOptions) so it can be stubbed in tests.apps/nvt-bridge/src/nvt/docker.test.ts#L1-L118: once injectable, add tests for done-detection across chunked/partial stdout lines, timeout expiry, and abort-signal cancellation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/nvt-bridge/src/nvt/docker.ts` around lines 115 - 169, Make waitForDone testable by introducing an injectable SpawnFn through DockerNvtDriverOptions, mirroring the existing ExecFn seam, and use it instead of calling imported spawn directly in waitForDone. In apps/nvt-bridge/src/nvt/docker.ts lines 115-169, preserve the existing streaming, timeout, abort, and exit behavior. In apps/nvt-bridge/src/nvt/docker.test.ts lines 1-118, add tests using the injected spawn stub for done detection across chunked or partial stdout lines, timeout expiry, and abort-signal cancellation.
🤖 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 `@agents/nvt-fat-developer/.env.example`:
- Around line 28-38: Remove the concrete credential from ANTHROPIC_AUTH_TOKEN in
agents/nvt-fat-developer/.env.example, leaving an empty placeholder for
deployment-time injection; also add .env to agents/nvt-fat-developer/.gitignore
so copied environment files are not tracked.
In `@agents/nvt-fat-developer/AGENTS.local.md.tmpl`:
- Around line 151-166: Add agents/nvt-fat-developer/AGENTS.local.md.tmpl to the
CODEOWNERS-protected paths and update the auto-merge sensitivity predicate in
this template’s auto-merge instructions to explicitly include that exact path.
Preserve the existing reviewer, audit-comment, and label workflow for
non-sensitive changes.
In `@apps/nvt-bridge/.env.example`:
- Around line 35-59: Declare documented PIPELINE_ROOT and RESTART_POLICY entries
in apps/nvt-bridge/.env.example, replace the personal NVT_ROOT value with a
<bruker> placeholder, and update apps/nvt-bridge/docker-compose.yml lines 18-29
to require both root variables with `${VAR:?...}` guards so startup fails when
either is unset.
In `@apps/nvt-bridge/package.json`:
- Around line 7-13: Raise the engines.node minimum in package.json from >=22.6
to >=22.18.0 so the start, test, and typecheck scripts run on a runtime with
default type stripping, or explicitly document a later required runtime if that
is the project’s chosen policy.
In `@apps/nvt-bridge/src/bridge.ts`:
- Around line 155-163: Propagate the abort signal from run() into handleEvent
and pass it as the signal option to driver.waitForDone(). Ensure an abort
produces the existing timeout-style outcome so handleEvent still writes the
fallback result line before shutdown.
In `@apps/nvt-bridge/src/nvt/docker.ts`:
- Around line 200-212: Update isDoneEvent to verify the parsed JSON value is a
non-null object before accessing obj.type or obj.event, returning false for null
and other non-object values; also add an isDoneEvent("null") case alongside the
existing invalid-input tests.
---
Nitpick comments:
In `@apps/nvt-bridge/src/nvt/docker.ts`:
- Around line 115-169: Make waitForDone testable by introducing an injectable
SpawnFn through DockerNvtDriverOptions, mirroring the existing ExecFn seam, and
use it instead of calling imported spawn directly in waitForDone. In
apps/nvt-bridge/src/nvt/docker.ts lines 115-169, preserve the existing
streaming, timeout, abort, and exit behavior. In
apps/nvt-bridge/src/nvt/docker.test.ts lines 1-118, add tests using the injected
spawn stub for done detection across chunked or partial stdout lines, timeout
expiry, and abort-signal cancellation.
In `@apps/nvt-bridge/src/topic.ts`:
- Around line 62-73: Update instanceNameFor to validate that the prefix,
separator, and hash can fit within opts.maxLength before constructing the
result; when the fixed prefix-hash portion exceeds the limit, fail fast with a
clear error instead of returning an overlong name. Preserve the existing slug
truncation behavior when sufficient room remains.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ffa42ab8-e1f2-4637-83d2-ab2458a79195
⛔ Files ignored due to path filters (1)
apps/nvt-bridge/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (33)
agents/nvt-fat-developer/.env.exampleagents/nvt-fat-developer/.gitattributesagents/nvt-fat-developer/.gitignoreagents/nvt-fat-developer/AGENTS.local.md.tmplagents/nvt-fat-developer/README.mdagents/nvt-fat-developer/triggers/.gitkeepapps/nvt-bridge/.env.exampleapps/nvt-bridge/.gitignoreapps/nvt-bridge/Dockerfileapps/nvt-bridge/README.mdapps/nvt-bridge/docker-compose.ymlapps/nvt-bridge/package.jsonapps/nvt-bridge/src/bridge.test.tsapps/nvt-bridge/src/bridge.tsapps/nvt-bridge/src/config.tsapps/nvt-bridge/src/index.tsapps/nvt-bridge/src/nvt/docker.test.tsapps/nvt-bridge/src/nvt/docker.tsapps/nvt-bridge/src/nvt/driver.tsapps/nvt-bridge/src/nvt/dryrun.tsapps/nvt-bridge/src/nvt/fake.tsapps/nvt-bridge/src/prompt.tsapps/nvt-bridge/src/scheduler.test.tsapps/nvt-bridge/src/scheduler.tsapps/nvt-bridge/src/state.test.tsapps/nvt-bridge/src/state.tsapps/nvt-bridge/src/topic.test.tsapps/nvt-bridge/src/topic.tsapps/nvt-bridge/src/triggers.test.tsapps/nvt-bridge/src/triggers.tsapps/nvt-bridge/src/types.tsapps/nvt-bridge/tsconfig.jsondoc/log.md
| # AUTH_TOKEN er fat-devs EGEN konsument-nøkkel i gatewayens routes.json, slik | ||
| # at trafikken kan skilles i loggen og nøkkelen revokeres alene. Det ekte | ||
| # OAuth-tokenet bor kun i gatewayens .env og er aldri inne i denne | ||
| # containeren. Modell-allowlisten håndheves i gatewayen (fail closed) — | ||
| # agenten kan ikke velge en dyrere modell selv. | ||
| # | ||
| # ⚠️ Host-oppslaget må VERIFISERES i M0 (issue #96): nvt-runtimen kjører med | ||
| # `network_mode: service:docker`, så det er ikke gitt at | ||
| # host.docker.internal løses her. Er den ikke det, brukes gateway-IP-en. | ||
| ANTHROPIC_BASE_URL=http://host.docker.internal:8787 | ||
| ANTHROPIC_AUTH_TOKEN=fat-developer |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not publish or track the gateway authentication credential.
ANTHROPIC_AUTH_TOKEN is documented as a per-agent consumer key, yet the example assigns a concrete value; copied .env files are also not ignored despite line 1 claiming otherwise. This enables unauthorized gateway use and accidental credential commits.
agents/nvt-fat-developer/.env.example#L28-L38: use an empty placeholder and inject the real token at deployment.agents/nvt-fat-developer/.gitignore#L1-L4: add.env.
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 38-38: [UnorderedKey] The ANTHROPIC_AUTH_TOKEN key should go before the ANTHROPIC_BASE_URL key
(UnorderedKey)
📍 Affects 2 files
agents/nvt-fat-developer/.env.example#L28-L38(this comment)agents/nvt-fat-developer/.gitignore#L1-L4
🤖 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 `@agents/nvt-fat-developer/.env.example` around lines 28 - 38, Remove the
concrete credential from ANTHROPIC_AUTH_TOKEN in
agents/nvt-fat-developer/.env.example, leaving an empty placeholder for
deployment-time injection; also add .env to agents/nvt-fat-developer/.gitignore
so copied environment files are not tracked.
| ## Auto-merge av trygge PR-er | ||
|
|
||
| PR-er som **ikke** rører noen sti i `.github/CODEOWNERS` (agent-instrukser, | ||
| skills, Docker-filer, `integrations/src/`, `scripts/`, `.github/`) kan | ||
| merges uten menneskelig godkjenning — se `doc/pr-prosess.md`. Prosessen er: | ||
|
|
||
| 1. Kjør en reviewer-subagent på PR-diffen — ferske øyne, ikke samme | ||
| kontekst som skrev koden. | ||
| 2. Post reviewens funn og konklusjon som kommentar på PR-en | ||
| (`gh pr comment`) — kommentaren er audit-sporet. | ||
| 3. Er reviewen ren: sett labelen `auto-merge` | ||
| (`gh pr edit <nr> --add-label auto-merge`). En GitHub Action merger når | ||
| required checks er grønne. | ||
|
|
||
| Rører PR-en en sensitiv sti, er labelen virkningsløs (branch protection | ||
| krever code owner uansett) — utelat den og pek på PR-en i svaret som før. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Protect this instruction template before allowing auto-merge.
AGENTS.local.md.tmpl is currently outside CODEOWNERS, but this flow permits non-owned PRs to receive auto-merge. A change weakening this template could therefore merge without human ownership and affect every rendered agent. Add this exact path to CODEOWNERS and treat it as sensitive in the auto-merge predicate.
🤖 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 `@agents/nvt-fat-developer/AGENTS.local.md.tmpl` around lines 151 - 166, Add
agents/nvt-fat-developer/AGENTS.local.md.tmpl to the CODEOWNERS-protected paths
and update the auto-merge sensitivity predicate in this template’s auto-merge
instructions to explicitly include that exact path. Preserve the existing
reviewer, audit-comment, and label workflow for non-sensitive changes.
| # --- nvt-oppsettet (kalibreres mot M0-funn, se issue #96) --- | ||
| # Sjekkouten av nvt-agent. Make-flyten (agent-init/agent-up) kjøres herfra. | ||
| # MERK: når bridgen kjører i container, må denne stien være IDENTISK med | ||
| # stien på hosten — compose-bind-mounts løses av hostens daemon. | ||
| NVT_ROOT=/home/ole/src/nvt-agent | ||
|
|
||
| # Agent-type nvt skal initialisere instansene med. | ||
| NVT_AGENT_TYPE=claude | ||
|
|
||
| # Hvor agentens triggers/ er mountet INNE i instansen. Brukes i prompten som | ||
| # forteller agenten hvor resultatlinja skal. Mounten settes opp i nvt-configen | ||
| # (kalibreres mot M0-funn). | ||
| NVT_INSTANCE_TRIGGERS_PATH=/triggers | ||
|
|
||
| # Prefiks for instansnavn (compose-prosjekt blir agent-<navn>, og code-server | ||
| # rutes som http://<navn>.agent.localhost:4090). | ||
| NVT_INSTANCE_PREFIX=fatdev | ||
|
|
||
| # Maks lengde på instansnavnet. Holdes godt under DNS-etikettgrensen (63) — | ||
| # nvt bygger både compose-prosjektnavn og Traefik-vertsnavn av det. | ||
| NVT_INSTANCE_NAME_MAX=40 | ||
|
|
||
| # Sett til 1 for å kjøre uten nvt (fake-driver): logger hva som VILLE blitt | ||
| # kjørt, og skriver aldri en fabrikert suksess. Kun for tørrkjøring. | ||
| NVT_BRIDGE_DRY_RUN=0 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Undeclared PIPELINE_ROOT breaks the compose mounts. docker-compose.yml loads .env and interpolates ${PIPELINE_ROOT} (and ${RESTART_POLICY}), but .env.example never declares them, so a copied template expands them to empty strings and the bridge quietly binds and polls the wrong triggers directory.
apps/nvt-bridge/.env.example#L35-L59: add documentedPIPELINE_ROOTandRESTART_POLICYentries, and replace the personalNVT_ROOT=/home/ole/...value with a<bruker>placeholder.apps/nvt-bridge/docker-compose.yml#L18-L29: guard both roots with${VAR:?...}so an unset value fails startup instead of silently resolving to a relative path.
📍 Affects 2 files
apps/nvt-bridge/.env.example#L35-L59(this comment)apps/nvt-bridge/docker-compose.yml#L18-L29
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/nvt-bridge/.env.example` around lines 35 - 59, Declare documented
PIPELINE_ROOT and RESTART_POLICY entries in apps/nvt-bridge/.env.example,
replace the personal NVT_ROOT value with a <bruker> placeholder, and update
apps/nvt-bridge/docker-compose.yml lines 18-29 to require both root variables
with `${VAR:?...}` guards so startup fails when either is unset.
| "engines": { | ||
| "node": ">=22.6" | ||
| }, | ||
| "scripts": { | ||
| "start": "node --disable-warning=ExperimentalWarning src/index.ts", | ||
| "test": "node --test --disable-warning=ExperimentalWarning", | ||
| "typecheck": "tsc --noEmit" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Node.js version type stripping enabled by default without experimental-strip-types flag
💡 Result:
Yes, type stripping is enabled by default in modern versions of Node.js, and no experimental flag is required to use it [1][2]. As of the current date (July 29, 2026), type stripping has been a stable, default feature for some time [1][3]. The following timeline outlines its evolution: - Default Enablement: Type stripping was enabled by default starting in Node.js v23.6.0 and backported to v22.18.0 [1][2]. - Removal of Experimental Warning: The experimental warning associated with type stripping was removed in v24.3.0 and v22.18.0 [1]. - Stability: The feature was officially marked as stable in v25.2.0 and v24.12.0 [1][3]. When enabled, Node.js automatically executes TypeScript files by stripping erasable syntax (such as type annotations, interfaces, and import types) and running the remaining JavaScript [1][4]. This process is lightweight and does not perform type checking [1][2]. If you need to disable this behavior, you can use the --no-strip-types flag [1]. Note that more advanced TypeScript features requiring code generation (such as enums or parameter properties) still require the --experimental-transform-types flag, which remains experimental [2][3][5].
Citations:
- 1: https://nodejs.org/api/typescript.html
- 2: https://nodejs.org/docs/latest-v23.x/api/typescript.html
- 3: module: mark type stripping as stable nodejs/node#60600
- 4: https://nodejs.org/learn/typescript/run-natively
- 5: module: remove experimental warning from type stripping nodejs/node#58643
🌐 Web query:
node --test default file discovery pattern TypeScript .test.ts files
💡 Result:
The Node.js test runner, invoked with the --test flag, automatically discovers test files using a predefined set of glob patterns [1][2]. When Node.js is running with its built-in TypeScript support (which performs type stripping), the test runner includes.ts,.cts, and.mts extensions in its default discovery pattern [1][3]. The default patterns for TypeScript files are: - /.test.{cts,mts,ts} - /-test.{cts,mts,ts} - /_test.{cts,mts,ts} - /test-.{cts,mts,ts} - /test.{cts,mts,ts} - /test//*.{cts,mts,ts} These patterns are active by default unless the --no-strip-types flag is used [1][3]. Node.js's built-in TypeScript support performs lightweight type stripping and does not perform type checking; it also ignores tsconfig.json configurations [4]. If you require full TypeScript support (such as type checking or specific tsconfig settings), you would typically use a third-party loader or tool (e.g., tsx) [4] rather than the default discovery mechanism.
Citations:
- 1: https://nodejs.org/dist/latest/docs/api/test.html
- 2: https://nodejs.org/api/test.html
- 3: https://github.com/nodejs/node/blob/main/doc/api/test.md
- 4: https://nodejs.org/docs/latest/api/typescript.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package files =="
git ls-files '*package.json' | sed -n '1,120p'
echo
echo "== apps/nvt-bridge/package.json =="
if [ -f apps/nvt-bridge/package.json ]; then
nl -ba apps/nvt-bridge/package.json | sed -n '1,120p'
fi
echo
echo "== relevant source/test files presence =="
git ls-files 'apps/nvt-bridge**' | sed -n '1,200p'
echo
echo "== Dockerfile / toolchain mentions =="
rg -n --glob '!node_modules' --glob '!dist' --glob '!build' 'node|Dockerfile|engines|22\.18|23\.6|strip|typescript|test' . | sed -n '1,240p'Repository: digdir/digdir-ai-agents
Length of output: 343
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package.json =="
cat -A apps/nvt-bridge/package.json | sed -n '1,120p' || cat apps/nvt-bridge/package.json | sed -n '1,120p'
echo
echo "== nvt-bridge tracked files =="
git ls-files 'apps/nvt-bridge' | sed -n '1,240p'
echo
echo "== nvt-bridge source/test content summary =="
for f in $(git ls-files 'apps/nvt-bridge'); do
if [ -f "$f" ]; then
echo "--- $f"
wc -l "$f"
if [ "$f" = "apps/nvt-bridge/src/index.ts" ] || echo "$f" | grep -qE '\.test\.ts$|\.ts$'; then
sed -n '1,220p' "$f"
fi
fi
doneRepository: digdir/digdir-ai-agents
Length of output: 50379
Make Node’s declared floor match the type-stripped runtime.
src/index.ts runs .ts imports without --experimental-strip-types, and that flag is required until Node strips erasable TS by default. The declared range allows >=22.6, but the current Node 22 behavior in the Dockerfile is 24; raise the floor to Node’s default-strip version (>=22.18.0) or document that these scripts must run on a later runtime.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/nvt-bridge/package.json` around lines 7 - 13, Raise the engines.node
minimum in package.json from >=22.6 to >=22.18.0 so the start, test, and
typecheck scripts run on a runtime with default type stripping, or explicitly
document a later required runtime if that is the project’s chosen policy.
| await this.opts.driver.sendPrompt( | ||
| ref, | ||
| renderPrompt(event, topic, this.opts.instanceTriggersPath ?? "/triggers"), | ||
| ); | ||
| await this.opts.triggers.appendBridgeLog(event.id, "prompt injisert, venter på signal done"); | ||
|
|
||
| const outcome = await this.opts.driver.waitForDone(ref, { | ||
| timeoutMs: this.opts.promptTimeoutMs, | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
waitForDone gets no AbortSignal, so graceful shutdown can block for promptTimeoutMs (default 3600s).
run() awaits scheduler.idle() after abort (Line 101), but the in-flight waitForDone has no way to learn about the abort — the driver contract accepts signal (see apps/nvt-bridge/src/nvt/driver.ts lines 48-51) and it is never passed. With NVT_BRIDGE_PROMPT_TIMEOUT_SECONDS=3600, SIGTERM leaves the process hanging well past any container stop grace period, so it gets SIGKILLed — which is exactly the "event ends without a result line" failure the class invariant is built to prevent.
Plumb the run signal into handleEvent and pass it through, so an abort surfaces as a timeout-style outcome and the fallback line still gets written.
🛠️ Sketch of the plumbing
- const outcome = await this.opts.driver.waitForDone(ref, {
- timeoutMs: this.opts.promptTimeoutMs,
- });
+ const outcome = await this.opts.driver.waitForDone(ref, {
+ timeoutMs: this.opts.promptTimeoutMs,
+ signal: this.shutdownSignal,
+ });Store the signal passed to run() on the instance (or hand it to the scheduler handler) so handleEvent can forward it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/nvt-bridge/src/bridge.ts` around lines 155 - 163, Propagate the abort
signal from run() into handleEvent and pass it as the signal option to
driver.waitForDone(). Ensure an abort produces the existing timeout-style
outcome so handleEvent still writes the fallback result line before shutdown.
| export function isDoneEvent(line: string): boolean { | ||
| const trimmed = line.trim(); | ||
| if (trimmed === "") return false; | ||
| let parsed: unknown; | ||
| try { | ||
| parsed = JSON.parse(trimmed); | ||
| } catch { | ||
| return false; | ||
| } | ||
| const obj = parsed as { type?: unknown; event?: unknown }; | ||
| const name = typeof obj.type === "string" ? obj.type : obj.event; | ||
| return name === "plugin.agent.signal.done"; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
isDoneEvent crashes the whole bridge process on a bare null line.
JSON.parse("null") succeeds and returns null (no exception, so the catch at Line 206 never fires). The subsequent obj.type access at Line 210 then throws TypeError: Cannot read properties of null, and since this happens inside the child.stdout "data" handler in waitForDone (an EventEmitter callback, not inside any surrounding try/catch), it becomes an uncaught exception that can crash the entire node process — taking down every in-flight topic, not just this one. Given the subscribe output format is explicitly unverified per this file's own docstring, a stray null line is a realistic failure mode to guard against.
🐛 Proposed fix
const obj = parsed as { type?: unknown; event?: unknown };
- const name = typeof obj.type === "string" ? obj.type : obj.event;
+ if (typeof parsed !== "object" || parsed === null) return false;
+ const obj = parsed as { type?: unknown; event?: unknown };
+ const name = typeof obj.type === "string" ? obj.type : obj.event;
return name === "plugin.agent.signal.done";Add a test case for isDoneEvent("null") alongside the existing invalid-input tests.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function isDoneEvent(line: string): boolean { | |
| const trimmed = line.trim(); | |
| if (trimmed === "") return false; | |
| let parsed: unknown; | |
| try { | |
| parsed = JSON.parse(trimmed); | |
| } catch { | |
| return false; | |
| } | |
| const obj = parsed as { type?: unknown; event?: unknown }; | |
| const name = typeof obj.type === "string" ? obj.type : obj.event; | |
| return name === "plugin.agent.signal.done"; | |
| } | |
| export function isDoneEvent(line: string): boolean { | |
| const trimmed = line.trim(); | |
| if (trimmed === "") return false; | |
| let parsed: unknown; | |
| try { | |
| parsed = JSON.parse(trimmed); | |
| } catch { | |
| return false; | |
| } | |
| if (typeof parsed !== "object" || parsed === null) return false; | |
| const obj = parsed as { type?: unknown; event?: unknown }; | |
| const name = typeof obj.type === "string" ? obj.type : obj.event; | |
| return name === "plugin.agent.signal.done"; | |
| } |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/nvt-bridge/src/nvt/docker.ts` around lines 200 - 212, Update isDoneEvent
to verify the parsed JSON value is a non-null object before accessing obj.type
or obj.event, returning false for null and other non-object values; also add an
isDoneEvent("null") case alongside the existing invalid-input tests.
1 participant
Miljøuavhengig del av M1: ny utførende agent
nvt-fat-developerbak uendretfilkontrakt, med en deterministisk
nvt-bridgesom mapper topic → levendenvt-instans.
Refs #97(ikkeCloses— M1 lukkes først etter E2E-verifisering mot ektenvt-instanser).
Leveranser
1.
apps/nvt-bridge/(Node/TS, samme stack og testoppsett somintegrations/: native type-stripping, ingen byggesteg,node --test)agents/nvt-fat-developer/triggers/inbox.jsonlmed samme dedupe-regelsom i dag: id uten linje i
results.jsonl.topic=payload.origin.event_iduten delta-suffiks, ellers eventets egenid. Regelen er portet fra
scripts/agent-runner.ps1(Get-TopicKey) —inkludert at strippingen er ankret og skjer én gang, så
x-d1-d2→x-d1.state/topics.json.NVT_BRIDGE_MAX_PARALLEL).agent-downfor inaktive topics; workspacet beholdes.status:"error"og forklaring — aldri enfabrikert suksess.
status:"ok"kan kun komme fra agenten selv.2. All nvt-interaksjon bak
NvtDrivermedFakeNvtDriverfor tester og enDryRunNvtDriverfor tørrkjøring. Den ekte adapterensrc/nvt/docker.tsertynn og merket «kalibreres mot M0-funn» (#96) i et blokk-kommentarhode som
lister nøyaktig hva som er ubekreftet (make-mål, containernavn,
agentdctl subscribe-format). Ingen antakelser om compose-stier eller nettverkutover det planen sier; adapteren gjør ingen sti-oversetting.
3.
agents/nvt-fat-developer/:triggers/-kontrakt, README,.env.exampleog
AGENTS.local.md.tmpl— instruks-malen. Kodeagent-protokollen gjenbrukesordrett der den kan («kjenn din begrensning»,
agent/<navn>,gh pr create --base,Closes #<nr>, aldri merge, retro/KB-steget). To tingskiller, som spesifisert: ingen innboks-polling, og
agentdctl signal doneetter resultatlinja.4. 77 tester mot fake-implementasjonen (dedupe, topic-avledning,
serialisering per topic, fallback-linja) + oppføring i
doc/log.md.integrations/src/er urørt — verifisert medgit diff --stat -- integrations/. Ruta iAGENT_ROUTESer driftskonfig ogtas ved utrulling.
Review er kjørt (som prosessen krever)
En reviewer-subagent med ferske øyne gikk over diffen og fant 1 blocker og 4
majors — alle reelle, alle reprodusert, alle fikset i
fdc1faemedregresjonstest (62 → 77 tester). Funnene og verifikasjonen er postet som
kommentar på denne PR-en (audit-sporet). Kort:
try→ en feilende logg-skriving etterlot eventetuten resultatlinje (evig ubehandlet, poller i ring).
log-verdi utenfortriggers/+ skriving utenforkatalogen.
safeId()fantes, men var ubrukt i produksjonskode.appendResultantok avsluttende linjeskift. Agenten skriver linja frihånds;uten linjeskift limte vår append seg på dens linje og gjorde begge uleselige
— agentens suksess tapt.
en levende sesjon to ganger.
integrations matcher kun på
id, kunne en lurt agent postet falsk suksess ien annen tråd.
Designvalg jeg måtte ta (ikke avgjort i planen)
apps/nvt-bridge/framforintegrations/-søsken (planen sa «apps/eller …»). Egen app holder brua unna CODEOWNERS-stien
integrations/src/.lowercase. Topic-id-ene er lange (
github-digdir-digdir-ai-agents-97-c…er45 tegn), og nvt bygger både compose-prosjektnavn og Traefik-vertsnavn
(
<navn>.agent.localhost) av navnet — DNS-etiketter tar maks 63.Deterministisk med vilje: mister vi
state/topics.json, peker samme topicfortsatt på samme instans og dermed samme workspace.
container per topic»; brua køer per topic og trenger vakten eksplisitt,
ellers re-dispatcher neste polling et event som er under arbeid (det har
ennå ingen resultatlinje).
logs/<id>.bridge.logfor broens diagnostikk, så den ikkekolliderer med agentens egen
logs/<id>.log.En andre prompt inn i en levende sesjon som står midt i arbeidet er verre
enn en ærlig feilmelding med peker til instansen.
--experimental-strip-types-flagg: Node ≥ 23.6 stripper selv, menstrip-only-modus avviser TS parameter properties. Alle klasser bruker
derfor eksplisitte felt + tilordning i konstruktøren — samme stil som
integrations/src/, som unngår dem konsekvent.spawnmed argv-array, aldrishell: true),--externaler ubetinget på det ene injeksjonspunktet, ogingen hemmeligheter i logger, resultatlinjer eller state. Bot-kontonavnet er
holdt utenfor repoet;
.env.exampleinneholder kun gateway-konsumentnøkkelen(samme mønster som jr).
Til deg som reviewer
Ikke sett
auto-merge. PR-en rører**/Dockerfileog**/docker-compose.yml(CODEOWNERS), så labelen ville vært virkningsløsuansett — og det er forventet her.
CI dekker ikke de nye testene. Jeg la til en
nvt-bridge-jobb ici.yml, men GitHub avviste pushen: agent-tokenet manglerworkflow-scope.Endringen er derfor rullet tilbake, og
.github/er urørt. Foreslått tekst,til manuell innlegging etter
integrations-jobben:CODEOWNERS-gap å vurdere:
agents/*/CLAUDE.mdfanger ikkeagents/nvt-fat-developer/AGENTS.local.md.tmpl, som er agentens instruks.Framtidige endringer i den ville sluppet unna code-owner-review. Jeg har
ikke rørt CODEOWNERS selv — hvem som må godkjenne hva er en
governance-beslutning, ikke en ingeniørbeslutning.
Planen sier noe issue-teksten ikke sier:
doc/plans/nvt-agent-integrasjon.mdfinnes på
v2.0(delegeringsprompten hevdet at den manglet — den ble noksjekket mot
main). Jeg har fulgt planen, inkludert beslutning 3 fra2026-07-27: llm-gatewayen med subscription-OAuth, ikke LM Studio, som
modell-backend. Issue nvt M0: sandkasse-bevis — nvt-instans med bot-PAT-grant og LM Studio på WSL2 #96-teksten sier fortsatt LM Studio.
Ikke med i M1 (bevisst)
ingen håndheving, så samtidig code-server-innhopp og headless-kjøring i
samme topic kan kollidere i arbeidskopien. Dokumentert i begge README-er.
AGENTS.local.mdinn i instansen og mount avtriggers/hører til
agent-init-oppsettet og kalibreres mot M0. Til det er gjort vilE2E treffe fallback-linja — malen ligger her nå for å kunne reviewes og
versjoneres.
Refs #97
Summary by CodeRabbit