Review an Agent PR That Writes Tool JSON Into Runtime Config

A composite review scenario starts with a failed Friday staging deploy. In that scenario, the planner route wrote tool JSON over runtime config. In the diff, retry ceilings climbed and TLS verification vanished.

The useful change was a fixture loader meant for local tests. The harmful change was an unbounded merge sitting beside that loader. This review splits those changes before any approval reaches the branch.

What landed in the diff

The agent added three files and edited the shared config module. One route parsed a request body and called a recursive merge helper. A sample payload sat in fixtures while the helper copied every key.

Reviewers can trust the loader after path checks pass in tests. They should revert any merge that writes into live config. They should test the allowlist that replaces that unsafe merge.

1. Read the helper that writes config

Start the review at the function that writes runtime config. Short helpers often hide wide effects across the whole process. This helper is the center of the review, not a side note.

function deepMerge(target, patch) {
  for (const key of Object.keys(patch)) {
    const next = patch[key];
    if (next && typeof next === "object" && !Array.isArray(next)) {
      target[key] = deepMerge(target[key] ?? {}, next);
    } else {
      target[key] = next;
    }
  }
  return target;
}

Every key in the patch can land inside the target object. Nested objects receive that same copy behavior at each level. Arrays and scalar values replace whatever value stood there before.

2. Mark the route trust boundary

The route treats model tool output as a trusted config patch. That output is not a source file a human already reviewed. A planner can emit keys the original author never typed.

function applyPlannerPatch(runtimeConfig, body) {
  const patch = body.patch ?? {};
  deepMerge(runtimeConfig, patch);
  return { ok: true, config: runtimeConfig };
}

Revert this route or gate it behind a local test flag only. Never bind the route on a shared or internet-facing server. The returned object also exposes the full config to callers.

3. Separate the loader you can trust

The fixture loader is the part worth keeping after a path review. It should accept only simple names and resolve under a fixed root. It should reject traversal segments before any file read begins.

import { readFile } from "node:fs/promises";
import path from "node:path";

export async function loadFixture(name) {
  if (!/^[a-z0-9-]+$/.test(name)) {
    throw new Error("fixture name rejected");
  }
  const root = path.resolve("fixtures");
  const file = path.resolve(root, `${name}.json`);
  if (!file.startsWith(root + path.sep)) {
    throw new Error("fixture path rejected");
  }
  return JSON.parse(await readFile(file, "utf8"));
}

Trust this loader only when tests cover bad names and odd paths. Do not trust it if it accepts absolute paths from the request. Do not let the loader feed its JSON into the live merge.

The path check above is a proposal, not a finished library. Reviewers should confirm it on each target platform before trust.

4. Build the deny table before style notes

Build a deny table before any comment about naming or style. That table is the main review artifact for this pull request. It tells the author which keys to delete from the patch path.

Key path	Risk if a patch lands	Review action
retries.max	Extra model calls can spend the token budget	Revert and set the cap in code
tls.rejectUnauthorized	Staging calls can skip certificate checks	Revert before any shared run
auth.mode	A string can weaken session checks	Revert and cover with tests
log.level	Debug logs may store raw prompts	Keep the code default only
db.url	A patch can redirect later writes	Deny the key and alert

The earlier float-billing review covers a different failure mode. This table covers control flags rather than currency math. Keep those two checklists in separate comments on the request.

5. Replace the merge with an allowlist

Allow only keys that a human already named in source control. Ignore every unknown key and record it for the review log. Return that ignored list beside the updated config snapshot.

const ALLOWED = new Set(["jobs.timeoutMs", "jobs.batchSize"]);

export function applyAllowed(config, patch, allowed = ALLOWED) {
  const ignored = [];
  for (const [key, value] of Object.entries(flatten(patch))) {
    if (!allowed.has(key)) {
      ignored.push(key);
      continue;
    }
    if (typeof value !== "number" || value < 1 || value > 30000) {
      ignored.push(key);
      continue;
    }
    setPath(config, key, value);
  }
  return { config, ignored };
}

Numeric values must stay inside a fixed inclusive range. String values never pass through this replacement helper. Nested control flags stay outside the allowed key set.

The flatten and setPath helpers belong in that same module. Reviewers should read both helpers before they approve the change. A careless flatten can still smuggle unexpected keys past a hasty check.

export function flatten(obj, prefix = "", out = {}) {
  for (const [key, value] of Object.entries(obj ?? {})) {
    const next = prefix ? `${prefix}.${key}` : key;
    if (value && typeof value === "object" && !Array.isArray(value)) {
      flatten(value, next, out);
    } else {
      out[next] = value;
    }
  }
  return out;
}

export function setPath(config, dotted, value) {
  const parts = dotted.split(".");
  let node = config;
  for (const part of parts.slice(0, -1)) {
    node[part] = node[part] ?? {};
    node = node[part];
  }
  node[parts.at(-1)] = value;
  return config;
}

6. Add tests the agent did not show

The tests below are proposed examples and were not executed here. They import the node test runner and the strict assert module. Run the file on a disposable runner before anyone merges the branch.

import test from "node:test";
import assert from "node:assert/strict";
import { applyAllowed } from "./apply-config.js";

test("rejects tls and retry overrides", () => {
  const config = {
    tls: { rejectUnauthorized: true },
    retries: { max: 2 },
    jobs: { timeoutMs: 1000 }
  };
  const patch = {
    tls: { rejectUnauthorized: false },
    retries: { max: 99 },
    jobs: { timeoutMs: 2500 }
  };
  const allowed = new Set(["jobs.timeoutMs"]);
  const result = applyAllowed(config, patch, allowed);
  assert.equal(config.tls.rejectUnauthorized, true);
  assert.equal(config.retries.max, 2);
  assert.equal(config.jobs.timeoutMs, 2500);
  assert.deepEqual(result.ignored.sort(), [
    "retries.max",
    "tls.rejectUnauthorized"
  ]);
});

test("drops unknown batch size outside the allowlist", () => {
  const config = { jobs: { timeoutMs: 1000 } };
  const patch = { jobs: { timeoutMs: 2500, batchSize: 4 } };
  const allowed = new Set(["jobs.timeoutMs"]);
  applyAllowed(config, patch, allowed);
  assert.equal(config.jobs.timeoutMs, 2500);
  assert.equal(config.jobs.batchSize, undefined);
});

A green run means the allowlist held for these two cases. It does not prove that every future key will stay safe. Add a new case whenever a fresh flag enters the config module.

Use these commands on the review branch, not on production hosts. The first command runs the file once with the default reporter. The second command prints a spec report for the pull request thread.

node --test test/apply-config.test.js
node --test --test-reporter=spec test/apply-config.test.js

7. Draft on a free runner, then decide locally

Disclosure: This article was prepared as part of MonkeyCode's product outreach. The outreach brief states free model access plus a free server option. It also states a free allowance of 10 million tokens.

Confirm the live terms before a review depends on that allowance. This draft assumes no model names, hardware specs, or fixed duration. Quotas and server access can change, so the live page wins.

A reviewer can clone the branch onto that free server option. The free model may draft a first comment from the deny table. A human still decides what to revert and what to keep.

A model draft remains a suggestion rather than a merge approval. Paste test output under the comment, not runtime config or tokens. Keep secrets and customer rows out of fixtures and prompts.

Fetch the pull request head into a local review branch first. Switch to that branch, then run the proposed test file. The branch name and pull number below are examples, not a live request.

git fetch origin pull/184/head:review-184
git switch review-184
node --test test/apply-config.test.js

8. Leave a four-line review comment

Write the review comment as four short ordered lines. Authors often skim long essays and miss the required revert. These lines should match the trust split from this review.

1. Trust the fixture loader after its path tests pass cleanly.
2. Revert the deep merge into runtime config and the public route.
3. Replace that merge with the allowlist and the deny table.
4. Require both proposed test cases before the next push.

Add a note if the agent retries model calls inside the route. That budget bug belongs in a separate ticket, not this one. Do not bury that budget issue under the merge discussion.

9. Watch three failure shapes

Watch for three repeated failure shapes in agent config diffs. A boolean buried under tls can disable certificate checks. A numeric retry field can spend a token allowance quickly.

A string auth mode can weaken session checks without a type error. Reproduction for this review stays local and non-destructive. Copy the bad patch into a fixture, then call the allowlist helper.

Assert that denied keys leave the original config values untouched. Store that fixture beside the tests so the next reviewer can rerun it.

{
  "tls": { "rejectUnauthorized": false },
  "retries": { "max": 99 },
  "jobs": { "timeoutMs": 2500 }
}

Limits and who should skip this

This allowlist fits flat numeric job settings and nothing broader. It is the wrong tool for secrets, URLs, or role names. Teams that already sign config bundles should verify those signatures instead.

A single-file prototype does not need this planner route at all. Do not use this pattern when patches come from tenant input. Do not use it to hot-edit flags on a production process.

Do not treat a model draft as the final human reviewer. Skip the free runner when the repository holds customer data. Skip it when legal review forbids sending code to a hosted model.

One optional next step

Readers who want a spare runner can check current MonkeyCode terms. Confirm the free server option and the stated token allowance first. Then run the proposed tests and keep the verdict in human hands.

Closing check

Approve the request only after the deep merge helper is gone. Keep the fixture loader when tests pin its file paths. Record ignored keys in the pull request thread for the next reviewer.

That record makes a repeated mistake easier to spot later. Ship the allowlist only after the proposed tests pass on the review branch.