[paynejd]

What changed and why

During AutoMatch, fetchRecommendation logs each AI Assistant call in the row's history. Three of its six log() calls passed no row:

• AIRecommendationLimitReached when the response is a quota or preview-limit error;
• AIRecommendation with extras.error when the AI Assistant answers with detail;
• AIRecommendationLimitReached when the call throws a quota or preview-limit error.

log() then put the entry on the row open in the side panel, or under a null key when no row was open. So the failure or quota stop showed up on another row, and the row the AI was called for showed nothing. The same happened outside a run, when the AI retry button in the grid was used on one row while another row was open. The three calls now pass __index, as the other three AI entries already did.

Superseded calls still log on their row. The ticket asked whether a call whose run was superseded should log at all. It should. isSuperseded() guards the run's bookkeeping (the row's stage and the failure count, which now belong to the newer run). It doesn't guard the row's history: a superseded call's result entry and its failed-attempt entries were already logged on its row, and so was its analysis. Both limit-reached sites now match: handlePreviewLimitError still gets isSuperseded() ? null : __index for the stage, and the log entry always goes on __index. The comment above isSuperseded now says so. A new comment at log() says that work on a row other than the open one must pass its index.

How it was tested

• New test src/components/map-projects/__tests__/aiRecommendationLogs.test.js. It parses MapProject.jsx with @babel/core (already a devDependency), finds every log() call in fetchRecommendation (all six are AIRecommendation* entries), and checks that each one passes __index.
  • Against origin/main's MapProject.jsx it fails, listing exactly the three calls above (lines 5845, 5852 and 5885, each with no row).
  • At the PR's head it passes.
• Why the test reads source: fetchRecommendation is a closure over about forty pieces of MapProject state. The test runner has no JSX transform, and the integration tests mirror patterns rather than mount MapProject, so no test here can run a bulk AutoMatch. Extracting the closure just to drive it would be a far bigger change than this fix. runBulkAIAnalysis calls fetchRecommendation(row, template, true), and __index is that row's index, so the check covers each row of a bulk run, and single-row calls too.
• The pr.yml gate, run at the PR's head on Node 22.23.2:
  • secret check on start-prod.sh: pass;
  • npm ci: pass;
  • ESLint over src: exit 0. Locally I ran it with --no-eslintrc -c .eslintrc.json, because an outer checkout's .eslintrc.json otherwise cascades in; that is the same config CI uses;
  • npm run test:all: 448 unit tests and 6 integration tests, all pass.
• PR checks: "Lint and test" passed at the head (run 38291993532).
• No visual change, so no browser check.

Acceptance criteria

• Pass __index as the second argument in the three calls, as the other three do. See the three log() calls in fetchRecommendation.
• Decide whether a superseded call should log at all, and make the two limit-reached sites consistent. Decision: it logs on its own row (above). Both sites now pass isSuperseded() ? null : __index to handlePreviewLimitError and __index to log(). The test enforces the second part: a conditional row would fail it.
• Add a test: during a bulk run, every AIRecommendation* entry is recorded under the row being analysed. aiRecommendationLogs.test.js checks this in fetchRecommendation's source rather than by running a bulk run, for the reason given above. Codex flagged that gap as a P2 (F1.1); see below.

Review

• Codex pass 1, on 9a1a554: one P2 and no P0 or P1. F1.1 says the test reads source instead of running a bulk run. Accepted: a test that runs the AI step needs fetchRecommendation extracted, or MapProject mounted under test.
• Simplification review: four cuts, all applied in 885c64b. The test now checks every log() call in fetchRecommendation, with no helper and a shorter header, and the log() comment is shorter.
• Codex check of the cuts, on 885c64b: clean.

Nothing is open: no P0 or P1, and F1.1 is the one accepted P2.

Docs

No doc describes row logs: there is no docs/, and the README doesn't cover them. So the code comments are the update: one at log() and one above isSuperseded in fetchRecommendation.

Not done, on purpose

• No test that runs the AI step (Codex F1.1). It would need fetchRecommendation extracted from MapProject, or MapProject mounted under test. Either is a refactor beyond this ticket.
• Entries already misfiled in saved projects stay where they are. Nothing recorded which row they were for, so they can't be moved back.
• log() keeps its fallback to the open row. User actions on the open row (mapping, decisions, comments) rely on it.

Noticed, not fixed

Nothing beyond the ticket.

closes OpenConceptLab/ocl_issues#2887

🤖 Generated with Claude Code

https://claude.ai/code/session_01AEzMedE6q8ivk6uh6KeHQz

Written by an Octoad session worker (worker-v0.11, recipe lean-loop, flow public>public, run openconceptlab-ocl-issues-2887-20261012-141032).

[paynejd]

Codex review, pass 1 (codex-cli 0.162.1, commit 9a1a554)

1 finding: 1 P2.

• F1.1 · P2 · src/components/map-projects/tests/aiRecommendationLogs.test.js:43 · The source assertion catches the three omitted arguments but does not meet acceptance criterion 3's bulk-run behavior test. (With bulk rows 1 and 2 and panel row 0 or null, neither recommendation execution nor the resulting row histories are tested. An in-memory check confirmed this assertion still passes when the assignment '__index = _row.__index' is replaced with '__index = rowIndex', recreating incorrect attribution despite every call passing the named variable.)

findings.json

{
  "reviewer": "codex-cli 0.162.1",
  "perspective": "defects",
  "mode": "full",
  "commit": "9a1a5541f21aa61fa6677c1f3f0a75294565fdcb",
  "verdict": "findings",
  "prior": [],
  "findings": [
    {
      "id": "F1.1",
      "severity": "P2",
      "tag": null,
      "file": "src/components/map-projects/__tests__/aiRecommendationLogs.test.js",
      "line": 43,
      "claim": "The source assertion catches the three omitted arguments but does not meet acceptance criterion 3's bulk-run behavior test.",
      "scenario": "With bulk rows 1 and 2 and panel row 0 or null, neither recommendation execution nor the resulting row histories are tested. An in-memory check confirmed this assertion still passes when the assignment '__index = _row.__index' is replaced with '__index = rowIndex', recreating incorrect attribution despite every call passing the named variable.",
      "action": "Add behavioral coverage executing the production recommendation and logging logic with mocked AI responses for the three affected branches. Assert entries land under the analysed rows with another row open and with no row open, and cover the chosen superseded-call behavior.",
      "lines_delta": null,
      "confidence": "high"
    }
  ]
}

[paynejd]

Response to Codex pass 1

• F1.1 accepted: a test that runs the AI step needs fetchRecommendation (a closure over about forty pieces of MapProject state) extracted, or MapProject mounted under test, and either is a refactor well beyond this ticket. The __index assignment in the scenario also picks the row the request sends, the stage it marks and the analysis it stores, so breaking it would show far beyond the log.

No P0 or P1 is open, so the Codex loop stops after this pass.

[paynejd]

Simplification review (commit 9a1a554)

4 cuts: 3 shrink, 1 yagni.

• S1 · shrink · src/components/map-projects/tests/aiRecommendationLogs.test.js:30 · the AIRecommendation* action filter and the guards around it (all six log() calls in fetchRecommendation are AI calls) → check every log() call in fetchRecommendation (-4)
• S2 · yagni · src/components/map-projects/tests/aiRecommendationLogs.test.js:20 · the one-caller getAILogCalls wrapper and the module-level source → read, parse and walk in the test body (-7)
• S3 · shrink · src/components/map-projects/tests/aiRecommendationLogs.test.js:2 · header lines restating log()'s fallback and the bug's symptom → two short paragraphs (-3)
• S4 · shrink · src/components/map-projects/MapProject.jsx:1718 · the comment's restatement of log()'s default → a two-line rule for callers (-1)

findings.json

{
  "reviewer": "claude subagent",
  "perspective": "simplify",
  "mode": "full",
  "commit": "9a1a5541f21aa61fa6677c1f3f0a75294565fdcb",
  "verdict": "findings",
  "prior": [],
  "findings": [
    {
      "id": "S1",
      "severity": null,
      "tag": "shrink",
      "file": "src/components/map-projects/__tests__/aiRecommendationLogs.test.js",
      "line": 30,
      "claim": "The action filter and the guards around it (entry?.type !== 'ObjectExpression', property.key?.name, ?.value?.value, String(action), row?.name || null) narrow the walk to AIRecommendation* calls. But all six log() calls in fetchRecommendation are AIRecommendation* calls, so the filter keeps every one, and the guards cover argument shapes no call there has. Checked in a scratch copy: the walk without the filter finds the same six calls, flags the same three on origin/main (lines 5845, 5852, 5885) and flags none at HEAD.",
      "scenario": null,
      "action": "Check every log() call in fetchRecommendation: `CallExpression({node}) { if(node.callee.name === 'log') calls.push({line: node.loc.start.line, row: node.arguments[1]?.name}) }`. Keep the block body: Babel throws on a visitor that returns a value, so `=> … && calls.push(…)` fails. Change the assert.ok message to 'no log() calls found in fetchRecommendation'.",
      "lines_delta": -4,
      "confidence": "high"
    },
    {
      "id": "S2",
      "severity": null,
      "tag": "yagni",
      "file": "src/components/map-projects/__tests__/aiRecommendationLogs.test.js",
      "line": 20,
      "claim": "getAILogCalls has one caller, the file's only test, and the module-level `source` is read only by it. The wrapper adds a comment, a declaration, a return, a closing brace and a call line.",
      "scenario": null,
      "action": "Do the read, parse and walk in the test body: `const source = readFileSync(…)`, `const calls = []`, `babel.traverse(babel.parseSync(source, {…}), {VariableDeclarator(path) { if(path.node.id.name === 'fetchRecommendation') path.traverse({…}) }})`, then the two asserts. With S1 and S3 this gives a 32-line file. A scratch copy of it passes against HEAD's MapProject.jsx and fails against origin/main's on the three calls this PR fixed.",
      "lines_delta": -7,
      "confidence": "medium"
    },
    {
      "id": "S3",
      "severity": null,
      "tag": "shrink",
      "file": "src/components/map-projects/__tests__/aiRecommendationLogs.test.js",
      "line": 2,
      "claim": "Lines 3-5 of the header explain what log() does with an entry passed without a row, which the comment this PR adds above log() in MapProject.jsx now says. They also describe the bug's symptom, which the ticket already records. 'In a bulk run that's the run's row.' (line 10) restates the sentence before it.",
      "scenario": null,
      "action": "Keep two short paragraphs. First: 'Every entry fetchRecommendation logs goes on the row it analysed, not the row open in the side panel (OpenConceptLab/ocl_issues#2887).' Second: the paragraph on why the test reads source, ending '…each log() call in it must pass __index, the row it was called for.'",
      "lines_delta": -3,
      "confidence": "medium"
    },
    {
      "id": "S4",
      "severity": null,
      "tag": "shrink",
      "file": "src/components/map-projects/MapProject.jsx",
      "line": 1718,
      "claim": "The new comment above log() opens by restating its signature and default: 'index is the row the entry is about. Without it the entry goes on the row open in the side panel' says what `index === undefined ? rowIndex : index` says. Only two things in it are new: the rule for callers, and that rowIndex is the row open in the side panel.",
      "scenario": null,
      "action": "Two lines: `// Work on a row other than the one open in the side panel (a run's, a grid` and `// button's) must pass its index (OpenConceptLab/ocl_issues#2887).`",
      "lines_delta": -1,
      "confidence": "low"
    }
  ]
}

[paynejd]

Response to the simplification review

• S1 applied in 885c64b
• S2 applied in 885c64b
• S3 applied in 885c64b
• S4 applied in 885c64b

The slimmer test still fails against origin/main's MapProject.jsx on the same three calls (lines 5845, 5852 and 5885), and passes at 885c64b. I reran the full gate (secret check, npm ci, ESLint, npm run test:all with 448 unit and 6 integration tests), and everything passes.

[paynejd]

Codex check of the cuts (commit 885c64b)

Clean. The check covered 9a1a554..885c64b, the simplification cuts S1 to S4; nothing to revert.

findings.json

{
  "reviewer": "codex-cli 0.162.1",
  "perspective": "defects",
  "mode": "cuts",
  "commit": "885c64b",
  "verdict": "clean",
  "prior": [],
  "findings": []
}