Skip to content

Etapa 2: edicion de salidas entre mapas - #94

Open
xusuxiang8 wants to merge 1 commit into
Bitcoindefi:mainfrom
xusuxiang8:feat/issue10-map-exits
Open

Etapa 2: edicion de salidas entre mapas#94
xusuxiang8 wants to merge 1 commit into
Bitcoindefi:mainfrom
xusuxiang8:feat/issue10-map-exits

Conversation

@xusuxiang8

Copy link
Copy Markdown

Closes #10

Cambios

  • CRUD de salidas en specials.json (crear, leer, actualizar, borrar)
  • Creacion de ida y vuelta (round-trip) en una operacion
  • Validacion: mapa destino existe, tile destino no bloqueado
  • Deteccion de mapas inalcanzables (BFS desde mapa 1)
  • Deteccion de salidas huerfanas (apuntan a mapas que ya no existen)
  • Tests de integracion

API endpoints

  • GET /admin/game-data/maps/:mapNum/exits - listar salidas + entrantes
  • PUT /admin/game-data/maps/:mapNum/exits/:x/:y - crear/actualizar salida
  • POST /admin/game-data/maps/:mapNum/exits/:x/:y/round-trip - crear ida y vuelta
  • DELETE /admin/game-data/maps/:mapNum/exits/:x/:y - borrar salida
  • GET /admin/game-data/maps/inaccessible - detectar mapas inalcanzables
  • GET /admin/game-data/maps/orphan-exits - detectar salidas huerfanas

…tcoindefi#10)

- CRUD de salidas en specials.json
- Creacion de ida y vuelta en una operacion
- Validacion: mapa destino existe, tile no bloqueado
- Deteccion de mapas inalcanzables (BFS desde mapa 1)
- Deteccion de salidas huerfanas
- Tests de integracion
Comment thread api/src/server.ts
return;
}

const mapsSourceDir = require("path").join(__dirname, "mapas_source");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚨 Bug: Write/diagnostic exit endpoints use wrong mapas_source path

The GET /exits handler resolves the maps directory as join(__dirname, "mapas_source") (correct: compiled server.js sits in dist/, and the build copies src/mapas_source to dist/mapas_source; in ts-node __dirname is api/src which also contains mapas_source). But the PUT, POST round-trip, DELETE, inaccessible and orphan-exits handlers use join(__dirname, "..", "mapas_source"), which resolves to api/mapas_source — a directory that does not exist in either dev or prod. As a result createOrUpdateExit/createRoundTripExit always fail validation (map_not_found), deleteExit returns exit_not_found, and findInaccessibleMaps/findOrphanExits always return empty. Remove the ".." segment so all six handlers use join(__dirname, "mapas_source"), matching the working GET handler.

Use the same path resolution as the GET /exits handler in all five other endpoints (drop the ".." segment).:

const mapsSourceDir = require("path").join(__dirname, "mapas_source");
  • Apply fix

Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎

Comment thread api/src/server.ts
const x = Number.parseInt(request.params.x ?? "", 10);
const y = Number.parseInt(request.params.y ?? "", 10);

if (![mapNum, x, y].every((n) => Number.isInteger(n) && n > 0)) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Edge Case: Exit endpoints reject valid coordinate 0 (n > 0 check)

validateExit accepts grid coordinates in the range 0..99 (MAP_SIZE-1), but the PUT and round-trip handlers validate params with Number.isInteger(n) && n > 0, and round-trip also applies n > 0 to destX/destY. This makes it impossible to create or update an exit on row 0 or column 0 (e.g. x=0 or y=0), returning a spurious 400. Validate x/y/destX/destY against the actual grid bounds (>= 0 and < MAP_SIZE) instead of > 0, keeping the > 0 check only for map numbers.

Allow coordinate 0; keep positivity check only for map number. validateExit still enforces the upper bound.:

if (!Number.isInteger(mapNum) || mapNum <= 0 ||
    !Number.isInteger(x) || x < 0 ||
    !Number.isInteger(y) || y < 0) {
    response.status(400).json({ error: "Parametros invalidos." });
    return;
}
  • Apply fix

Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎

Comment thread api/src/lib/mapExits.ts
Comment on lines +250 to +258
const specialsA = await loadSpecials(mapsSourceDir, mapA);
if (!specialsA.exits) specialsA.exits = {};
specialsA.exits[gridKey(ax, ay)] = { map: mapB, x: bx, y: by };
await saveSpecials(mapsSourceDir, specialsA);

const specialsB = await loadSpecials(mapsSourceDir, mapB);
if (!specialsB.exits) specialsB.exits = {};
specialsB.exits[gridKey(bx, by)] = { map: mapA, x: ax, y: ay };
await saveSpecials(mapsSourceDir, specialsB);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Bug: Round-trip exit creation is not atomic on write failure

createRoundTripExit validates both directions, then writes specialsA and specialsB in two separate awaited fs.writeFile calls. If the second write fails (disk error, permissions), the first exit is already persisted, leaving a dangling one-way exit and an inconsistent world state. Consider writing to temp files and renaming, or at least documenting/rolling back the first write if the second fails.

Fix:

await saveSpecials(mapsSourceDir, specialsA);
try {
    // ... build specialsB ...
    await saveSpecials(mapsSourceDir, specialsB);
} catch (err) {
    // rollback A
    delete specialsA.exits[gridKey(ax, ay)];
    await saveSpecials(mapsSourceDir, specialsA);
    throw err;
}
  • Apply fix

Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎

@gitar-bot

gitar-bot Bot commented Aug 19, 2026

Copy link
Copy Markdown
Code Review 🚫 Blocked 0 resolved / 3 findings

Adds map exit CRUD endpoints, validation, and reachability checks, but fails to use the correct data path in write/diagnostic handlers, incorrectly rejects coordinate 0, and lacks atomicity in round-trip writes.

🚨 Bug: Write/diagnostic exit endpoints use wrong mapas_source path

📄 api/src/server.ts:1066 📄 api/src/server.ts:1098 📄 api/src/server.ts:1131 📄 api/src/server.ts:1163 📄 api/src/server.ts:1186 📄 api/src/server.ts:1203

The GET /exits handler resolves the maps directory as join(__dirname, "mapas_source") (correct: compiled server.js sits in dist/, and the build copies src/mapas_source to dist/mapas_source; in ts-node __dirname is api/src which also contains mapas_source). But the PUT, POST round-trip, DELETE, inaccessible and orphan-exits handlers use join(__dirname, "..", "mapas_source"), which resolves to api/mapas_source — a directory that does not exist in either dev or prod. As a result createOrUpdateExit/createRoundTripExit always fail validation (map_not_found), deleteExit returns exit_not_found, and findInaccessibleMaps/findOrphanExits always return empty. Remove the ".." segment so all six handlers use join(__dirname, "mapas_source"), matching the working GET handler.

Use the same path resolution as the GET /exits handler in all five other endpoints (drop the ".." segment).
const mapsSourceDir = require("path").join(__dirname, "mapas_source");
⚠️ Edge Case: Exit endpoints reject valid coordinate 0 (n > 0 check)

📄 api/src/server.ts:1087 📄 api/src/server.ts:1126

validateExit accepts grid coordinates in the range 0..99 (MAP_SIZE-1), but the PUT and round-trip handlers validate params with Number.isInteger(n) && n > 0, and round-trip also applies n > 0 to destX/destY. This makes it impossible to create or update an exit on row 0 or column 0 (e.g. x=0 or y=0), returning a spurious 400. Validate x/y/destX/destY against the actual grid bounds (>= 0 and < MAP_SIZE) instead of > 0, keeping the > 0 check only for map numbers.

Allow coordinate 0; keep positivity check only for map number. validateExit still enforces the upper bound.
if (!Number.isInteger(mapNum) || mapNum <= 0 ||
    !Number.isInteger(x) || x < 0 ||
    !Number.isInteger(y) || y < 0) {
    response.status(400).json({ error: "Parametros invalidos." });
    return;
}
💡 Bug: Round-trip exit creation is not atomic on write failure

📄 api/src/lib/mapExits.ts:250-258

createRoundTripExit validates both directions, then writes specialsA and specialsB in two separate awaited fs.writeFile calls. If the second write fails (disk error, permissions), the first exit is already persisted, leaving a dangling one-way exit and an inconsistent world state. Consider writing to temp files and renaming, or at least documenting/rolling back the first write if the second fails.

Fix
await saveSpecials(mapsSourceDir, specialsA);
try {
    // ... build specialsB ...
    await saveSpecials(mapsSourceDir, specialsB);
} catch (err) {
    // rollback A
    delete specialsA.exits[gridKey(ax, ay)];
    await saveSpecials(mapsSourceDir, specialsA);
    throw err;
}
🤖 Prompt for agents
Code Review: Adds map exit CRUD endpoints, validation, and reachability checks, but fails to use the correct data path in write/diagnostic handlers, incorrectly rejects coordinate 0, and lacks atomicity in round-trip writes.

1. 🚨 Bug: Write/diagnostic exit endpoints use wrong mapas_source path
   Files: api/src/server.ts:1066, api/src/server.ts:1098, api/src/server.ts:1131, api/src/server.ts:1163, api/src/server.ts:1186, api/src/server.ts:1203

   The GET /exits handler resolves the maps directory as `join(__dirname, "mapas_source")` (correct: compiled server.js sits in dist/, and the build copies src/mapas_source to dist/mapas_source; in ts-node __dirname is api/src which also contains mapas_source). But the PUT, POST round-trip, DELETE, inaccessible and orphan-exits handlers use `join(__dirname, "..", "mapas_source")`, which resolves to api/mapas_source — a directory that does not exist in either dev or prod. As a result createOrUpdateExit/createRoundTripExit always fail validation (map_not_found), deleteExit returns exit_not_found, and findInaccessibleMaps/findOrphanExits always return empty. Remove the `".."` segment so all six handlers use `join(__dirname, "mapas_source")`, matching the working GET handler.

   Fix (Use the same path resolution as the GET /exits handler in all five other endpoints (drop the ".." segment).):
   const mapsSourceDir = require("path").join(__dirname, "mapas_source");

2. ⚠️ Edge Case: Exit endpoints reject valid coordinate 0 (n > 0 check)
   Files: api/src/server.ts:1087, api/src/server.ts:1126

   validateExit accepts grid coordinates in the range 0..99 (MAP_SIZE-1), but the PUT and round-trip handlers validate params with `Number.isInteger(n) && n > 0`, and round-trip also applies `n > 0` to destX/destY. This makes it impossible to create or update an exit on row 0 or column 0 (e.g. x=0 or y=0), returning a spurious 400. Validate x/y/destX/destY against the actual grid bounds (>= 0 and < MAP_SIZE) instead of `> 0`, keeping the `> 0` check only for map numbers.

   Fix (Allow coordinate 0; keep positivity check only for map number. validateExit still enforces the upper bound.):
   if (!Number.isInteger(mapNum) || mapNum <= 0 ||
       !Number.isInteger(x) || x < 0 ||
       !Number.isInteger(y) || y < 0) {
       response.status(400).json({ error: "Parametros invalidos." });
       return;
   }

3. 💡 Bug: Round-trip exit creation is not atomic on write failure
   Files: api/src/lib/mapExits.ts:250-258

   createRoundTripExit validates both directions, then writes specialsA and specialsB in two separate awaited fs.writeFile calls. If the second write fails (disk error, permissions), the first exit is already persisted, leaving a dangling one-way exit and an inconsistent world state. Consider writing to temp files and renaming, or at least documenting/rolling back the first write if the second fails.

   Fix:
   await saveSpecials(mapsSourceDir, specialsA);
   try {
       // ... build specialsB ...
       await saveSpecials(mapsSourceDir, specialsB);
   } catch (err) {
       // rollback A
       delete specialsA.exits[gridKey(ax, ay)];
       await saveSpecials(mapsSourceDir, specialsA);
       throw err;
   }

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Etapa 2: edicion de salidas entre mapas

1 participant