feat(ingest): name each embedded image ingest_document leaves out (#732, slice 1) by cdeust · Pull Request #893 · cdeust/Cortex

cdeust · 2026-11-05T18:33:22Z

Summary

ingest_document réduisait chaque image embarquée à un compteur anonyme (image_count dans le modèle, images_skipped dans le résultat) : un appelant ne pouvait pas savoir quelle capture de quelle section avait été perdue. Cause racine : les parseurs docx et Confluence comptaient les éléments image sur tout l'arbre (_count_images) sans en garder la référence ni la position.

Cette première tranche de #732 remplace ce compteur par un inventaire : chaque image devient un DocumentAttachment(ordinal, reference, section_index, state) dans l'ordre du document ; section_index est la position de la section dans ParsedDocument.sections (un indice plutôt que le texte du titre, pour ne pas recopier un long titre une fois par image). reference est ce que le balisage nomme (id de relation docx a:blip@r:embed / v:imagedata@r:id, ri:attachment@ri:filename / ri:url@ri:value ou img@src pour Confluence, "" sinon). Rien n'est encore extrait ni conservé, donc chaque image est rapportée unsupported. Le handler renvoie l'inventaire sous attachments à côté de images_skipped (qui en est la longueur) et section_headings, le titre de chaque section dans l'ordre que section_index indexe ; la notice et le résumé sont inchangés.

L'inventaire (core/document_images.py, partagé par les deux parseurs) compte une entrée par élément qui nomme une image (a:blip, v:imagedata, ri:attachment, ri:url, img) plus une par conteneur extérieur (w:drawing/w:pict, ac:image) qui n'en nomme aucun, lue sur l'attribut de cet élément, par un parcours itératif : la sortie est linéaire en l'entrée (mesuré : 40 000 w:pict imbriqués autour d'un id de 500 Ko → 1 entrée, 0,06 s ; 1 MiB de <img/> → 174 762 entrées, 14,2 Mo de JSON).

Refs #732

Tranches restantes (non couvertes ici) : conserver les octets, le digest et la version du document avec une référence de source accessible (état preserved-only) ; états extracted / failed explicites ; OCR/vision optionnels avec fournisseurs déclarés ; propriété des références partagées à la correction/suppression ; rejeu d'une fixture illustrée publique dans les deux hôtes.

Type of change

New feature (non-breaking change which adds functionality) — nouveau champ attachments dans le résultat ; images_skipped garde sa valeur.

Test plan

pytest tests_py/core tests_py/handlers tests_py/test_main.py : 5525 passed, 242 skipped, 1 failed — tests_py/core/test_wiki_coverage_dashboard.py::test_unreadable_page_is_skipped_not_fatal, qui échoue identiquement sur main (8ef4813) dans cet environnement (exécuté en root, un fichier chmod 000 reste lisible). Sans rapport avec ce diff.
Tests d'ingestion ciblés (parseurs, normaliseur, handler, writers, dédup) : 148 passed.
Nouveaux tests : référence docx (r:embed, v:imagedata@r:id), image dans un tableau, ordre et section des images, Confluence par nom de fichier / URL / src / aucune référence, inventaire exact renvoyé par le handler (tests_py/handlers/test_ingest_document_attachments.py).
ruff check / ruff format --check (ruff 0.17.3, version de uv.lock) sur les fichiers modifiés : OK. python scripts/check_craftsmanship.py : OK.

Mutation (mutmut 3.8.1, version de uv.lock) lancée localement sur les fonctions modifiées : aucun survivant hors des quatre équivalents enregistrés dans memory/mutation-equivalents.json (_local ×3, valeur initiale de pending), avec leur justification. Le script du gate (uv run) n'a pas pu s'exécuter ici (torch hors réseau autorisé). Pyright non lancé.

Audit notes

Couches : tout le parsing reste dans core/ (pur, stdlib xml.etree seulement) ; le handler ne fait que sérialiser (dataclasses.asdict).
Changement de comportement à noter : le comptage docx parcourt désormais les enfants de w:body au lieu de toute la racine w:document ; une image hors du corps (inexistante en pratique dans word/document.xml) ne serait plus comptée.

Breaking changes

ParsedDocument.image_count et NormalizedDocument.image_count (modèle interne core/) sont remplacés par attachments ; aucun appelant hors des tests. Le contrat MCP garde images_skipped.

Reviewer checklist

CHANGELOG.md non mis à jour (aucune PR autopilot ne le fait ; à trancher au merge).
Description du schéma ingest_document mise à jour.
No secrets / credentials / PII in the diff.

Generated by Claude Code

…, slice 1) ingest_document reduced every embedded image to an anonymous count (image_count / images_skipped), so a caller could not tell which screenshot of which section was lost. The docx and Confluence parsers now inventory each image in document order (DocumentAttachment: ordinal, the reference the markup names — docx relationship id, Confluence attachment filename/URL or img src — the heading of the section it sits in, and its state). Nothing is extracted or preserved yet, so every image is reported "unsupported"; the handler returns the inventory as `attachments` beside `images_skipped`, which is now its length. Refs #732 Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_018kyZYraH8hqphMRC2MG7Jz

…out (#732, slice 1)

cdeust · 2026-11-05T19:00:45Z

unit, run 42085372953 : échec causé par cette PR, corrigé dans cc6f4bf (fixup! de eea5b6b).

Ajouter attachments au résultat a fait entrer _ingestion_result dans le périmètre de la passe de mutation (ADR-1122), qui ne vérifie que les fonctions modifiées. Huit mutants préexistants y survivaient : aucun test ne fixait la valeur exacte de reason ("complete" / "document_ingestion_incomplete") ni le cas « composant manquant sans échec enregistré » (and not failures → or, mutant 24). tests_py/handlers/test_ingest_document_result.py fixe ingested, status, reason et idempotent_skip pour ces trois cas.

Vérifié localement avec mutmut 3.8.1 (la version de uv.lock), sur la même sélection de tests que CI plus le nouveau fichier : les 8 mutants de _ingestion_result sont tués. Les 7 survivants restants sont dans _ensure_wiki et handler, des fonctions que ce diff ne modifie pas ; CI les ignorait déjà sur ce run. Je n'ai pas encore la CI de cc6f4bf.

Generated by Claude Code

…ventory # Conflicts: # mcp_server/core/document_normalizer.py

chatgpt-codex-connector · 2026-11-06T13:51:03Z

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-11-06T16:23:34.339413Z	`7b47b04`	New commits
🔒 Security Review	✅ Completed 2026-11-06T16:28:52.057573Z	`7b47b04`	New commits

Security findings

Advisory findings (2)

🟡 Bound attachment metadata before returning it · Medium
🟡 Parse image references in a single pass · Medium

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

claude · 2026-11-06T13:51:49Z

Claude finished @cdeust's task in 3m 38s —— View job

Review of PR #893 finished

Gathered the diff and context
Ran five reviews (code quality, docs, security, performance, test coverage)
Checked each finding against the code
Posted the review: feat(ingest): name each embedded image ingest_document leaves out (#732, slice 1) #893 (comment)

Summary: one Blocker. Each image record copies the full section heading, and neither the number of images nor the reference length is limited. A document just under the 1 MiB input cap can therefore produce a result of about 20 GB. I traced this by reading the code. I did not reproduce it, because this job wasn't allowed to run Python.

There are also three Notes:

A hyperlinked image's reference picks up the hyperlink's id instead of the image's.
When Word saves a picture in both a modern and a fallback form, one picture is listed twice.
ADRs 0161, 0162 and 0163 still describe image_count.

Not done here: I didn't run lint, the craftsmanship gate or pytest (permission was denied in this job). The unit and regression jobs on 0763c5c show as cancelled, so the mutation gate on this commit is still unconfirmed.

Verdict: block

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0763c5c958

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-11-06T13:52:32Z

+    for node in image.iter():
+        for attr in (f"{{{_R_NS}}}embed", f"{{{_R_NS}}}id"):
+            value = node.get(attr)
+            if value:
+                return value


Select the image relationship instead of any descendant r:id

For a DOCX picture with a clickable hyperlink, wp:docPr/a:hlinkClick can occur before the picture's a:blip within w:drawing. This traversal returns the hyperlink's r:id rather than the image's r:embed, so ingest_document.attachments identifies the hyperlink relationship as the skipped image even though the correct image reference is available. Restrict relationship lookup to a:blip and legacy v:imagedata instead of accepting relationship attributes from arbitrary descendants.

Useful? React with 👍 / 👎.

Confirmé : dans wp:inline/wp:anchor, wp:docPr/a:hlinkClick@r:id précède a:blip@r:embed, donc le parcours renvoyait l'id du lien hypertexte pour une image cliquable. Corrigé dans f280f3d : seuls a:blip@r:embed et v:imagedata@r:id sont lus ; test test_a_picture_hyperlink_is_not_taken_for_the_image (plus les cas blip sans r:embed et imagedata sans r:id).

Generated by Claude Code

…out (#732, slice 1)

claude · 2026-11-06T13:55:26Z

Review 1 of 3: PR #893 at `0763c5c`

I ran five reviews (code quality, docs, security, performance, test coverage). They found one Blocker.

Blocker

The attachments result can grow quadratically from a 1 MiB document (mcp_server/core/docx_parser.py:103-110, mcp_server/core/confluence_parser.py:92-99, sent to the caller at mcp_server/handlers/ingest_document.py:283)

Every DocumentAttachment copies the full section_heading. The parsers don't limit how many images they list, and reference (for example a Confluence img@src data: URI) has no length limit either.
The only bound is the 1 MiB MAX_DOCUMENT_BYTES cap on the input (mcp_server/infrastructure/document_reader.py:20, ADR-0516). Nothing bounds the output.
Example: a docx document.xml of about 0.98 MiB passes the cap. It holds one Heading1 paragraph with a 500 KB heading, followed by 40,000 <w:drawing/> elements of 12 bytes each. That gives 40,000 attachments, each carrying the 500 KB heading, so the MCP result is about 20 GB of JSON.
In memory the records share one string object. The size blows up when asdict and the MCP layer serialize the result. A Confluence page from a third party can carry the same pattern with <img/>.
Before this PR the image output was a single integer, so this amplification is new in this diff.
I traced this by reading the code. I couldn't run Python in this job, so I have not reproduced it.
Possible fixes:
- Point to the section by index, or by its position in section_memory_ids, instead of repeating the heading text.
- Or put an explicit limit on the list and each string, and report what was cut (for example a truncated count). Don't truncate silently: CLAUDE.md rules out silent fallbacks.

Fix this →

Notes, no action before merge

docx_parser.py:86-90 returns the first r:embed or r:id found anywhere in the image's subtree. For a hyperlinked image, wp:docPr/a:hlinkClick@r:id comes before a:blip@r:embed, so the hyperlink's id is reported instead of the image's. Charts are reported the same way. Reading .//a:blip and .//v:imagedata directly avoids both, and also picks up r:link.
docx_parser.py:103: when Word writes mc:AlternateContent (a w:drawing in Choice and a w:pict in Fallback), one picture becomes two attachments, each with its own ordinal. Text boxes holding a picture add another.
docs/adr/ADR-0161/0162/0163 and their wiki/adr/migration/ mirrors still describe image_count and _count_images.

Other checks:

Layers and the # source: comment on ATTACHMENT_UNSUPPORTED are fine. The Confluence {urn:ri} namespace matches the wrapper at confluence_parser.py:25.
The image scan stays O(N), and the XML parser is unchanged.
The reviewers traced the mutation-gate targets for the new functions and found them pinned by exact assertions.
CI on 0763c5c: unit, regression, e2e and install show as cancelled, so the mutation gate on this head is still unconfirmed.

Verdict: block

claude · 2026-11-06T13:55:49Z

Claude finished @cdeust's task in 2m 4s —— View job

PR #893 review (1 of 3) done

Gather diff and context
Run five reviews (quality, security, performance, tests, docs)
Check Blockers and post the verdict

The review found no Blockers and the verdict is approve. The full review, including three Notes, is in this comment.

The main Note: one image that Word saves twice (a w:drawing with a w:pict fallback) is listed as two attachments. Worth fixing in a later slice. I could not run a script to reproduce it in this environment (tool permissions), so it comes from reading the code.

claude · 2026-11-06T13:57:57Z

Review 1 of 3 — PR #893 (head `f280f3d`)

I ran five reviews (quality, security, performance, test coverage, documentation) on git diff origin/main...HEAD. None found a Blocker: nothing in the diff breaks CI or security, or violates a CLAUDE.md rule.

Layers: parsing stays in pure core/, and the handler only serializes.
Constants: both new constants (ATTACHMENT_UNSUPPORTED, _R_NS) have a # source: comment.
Security: reference only ever appears in the tool result. It is never written to the wiki, a memory or SQL.
Performance: each top-level child is walked once, so the cost stays O(N).
Tests: they pin the ordinals, the image/img and drawing/pict members, the reference selection (including the hlinkClick case) and the state. No test reads image_count any more.

Notes, no action before merge

docx_parser._collect_images (mcp_server/core/docx_parser.py:107) takes every drawing/pict in the subtree, including nested ones. One image that Word saves as mc:Choice/w:drawing plus mc:Fallback/w:pict becomes two attachments, and a text box that holds a picture borrows the inner blip's id. The old counter already double-counted, but the inventory now shows the duplicates. Worth fixing in a later slice (skip mc:Fallback and nested drawing/pict). Found by reading the code; I could not run a reproduction here.
tests_py/handlers/test_ingest_document_result.py checks images_skipped but not attachments. test_ingest_document_attachments.py already pins the exact list.
Neither parser has a test for an image inside a heading paragraph. Reading the code, it gets the new section's heading, which is the correct result.

Verdict: approve

chatgpt-codex-connector

🛡️ Codex Security Review · Automatically triggered

Here are some automated security review suggestions for this pull request.

Reviewed commit: f280f3ddd8

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

You comment "@codex security review"
A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

chatgpt-codex-connector · 2026-11-06T14:02:17Z

        "retired_receipt_ids": written["retired_receipt_ids"],
-        "images_skipped": normalized.image_count,
+        "images_skipped": len(normalized.attachments),
+        "attachments": [asdict(a) for a in normalized.attachments],


🛡️ Codex Security Review · Automatically triggered

Security: Bound attachment metadata before returning it

When a user ingests an attacker-supplied Confluence export, this emits the full section_heading once per image without a response budget. A 16 KB file with a 10 KB heading and 1,000 <img/> nodes serializes to 10.1 MB; a file below the existing 1 MiB input cap can require over 45 GB before the stdio write, exhausting the Cortex or MCP-client process. Cap the attachment count and serialized size, and truncate or deduplicate headings before returning them. Cortex runs locally at the user's permission level, so the residual impact is local availability rather than a remote service outage.

SECURITY.md reference: SECURITY.md:L15-L20

Useful? React with 👍 / 👎.

Même cause que le Blocker de la revue Claude 1, corrigée dans f14f905 : une pièce jointe ne recopie plus le titre de sa section, elle porte section_index (indice dans ParsedDocument.sections) ; test test_a_long_heading_is_not_copied_once_per_image.

Ce qui reste : la sortie est désormais linéaire en l'entrée. reference est un texte distinct de l'entrée (jamais recopié), et chaque image coûte un objet de taille fixe. Mesuré sur f14f905 (Python 3.11, parse_confluence_storage + asdict + json.dumps) : 1 048 572 octets de <img/> répétés donnent 174 762 entrées et 14 219 379 octets de JSON, soit 13,6× l'entrée — amplification bornée, plus de croissance quadratique. Plafonner le nombre d'entrées avec un compteur truncated explicite est possible ; je laisse ce fil ouvert pour la décision du propriétaire.

Generated by Claude Code

…out (#732, slice 1)

claude · 2026-11-06T14:06:09Z

Claude finished @cdeust's task in 3m 23s —— View job

Review 3 of 3: PR #893 finished

Found the commits pushed after f280f3d (only f14f905)
Checked whether f14f905 fixed the earlier Blocker (only in part)
Ran five reviews (code quality, security, performance, test coverage, docs) on f14f905
Posted the review: feat(ingest): name each embedded image ingest_document leaves out (#732, slice 1) #893 (comment)

Summary: section_index stops the section heading from being copied for every image. The result can still grow quadratically, though. When image elements are nested (<w:pict> inside <w:pict>, or <ac:image> inside <ac:image>), each one is listed as an attachment and each copies the same deep reference. A 1 MiB input can still produce about 20 GB of JSON. I traced this by reading the code. I could not reproduce it, because this job wasn't allowed to write or run scripts.

Accepted: document_model.py, the handler and schema, the normalizer, the tests, and the hlinkClick fix.

Open Blocker: _collect_images and _image_reference in docx_parser.py and confluence_parser.py (introduced in eea5b6b).

Not done: I didn't run lint, pytest or mutmut, and I didn't check CI on f14f905, because this job wasn't allowed to.

Verdict: block

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f14f905ec0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-11-06T14:08:12Z

        "retired_receipt_ids": written["retired_receipt_ids"],
-        "images_skipped": normalized.image_count,
+        "images_skipped": len(normalized.attachments),
+        "attachments": [asdict(a) for a in normalized.attachments],


Expose the section mapping used by attachment indices

When a document contains image-only or headings-only sections, section_index cannot be resolved against the returned section_memory_ids: it indexes ParsedDocument.sections, whereas the normalizer excludes empty sections from section memories, and the parsed sections are never returned. For example, an image-only preamble followed by a populated heading produces attachment indices 0 and 1 but only one section memory; the preamble is also absent from the rendered wiki. Consequently, callers cannot reliably identify which section contains a skipped image. Return an index-to-section mapping alongside the inventory (once per section to avoid repeating long headings), or expose another resolvable section identifier.

Useful? React with 👍 / 👎.

Confirmé. Corrigé dans f0d8fd8 : le résultat porte section_headings, le titre de chaque section analysée dans l'ordre où section_index l'indexe, émis une seule fois (pas une fois par image) ; le test handler vérifie ["Design", "Rollout"].

Generated by Claude Code

chatgpt-codex-connector · 2026-11-06T14:08:12Z

+        if _local(node.tag) in ("drawing", "pict"):
+            out.append(
+                DocumentAttachment(
+                    ordinal=len(out) + 1,
+                    reference=_image_reference(node),


Inventory every image reference inside grouped pictures

For a DOCX containing grouped pictures, a single w:pict can contain a v:group with multiple v:shape/v:imagedata elements. The collector creates only one attachment for that container, and _image_reference returns immediately on its first image, silently omitting the remaining image references from the new inventory. Parsing a group with rId1 and rId2 reproduces this: only rId1 is returned. Collect the individual image-bearing elements within each container so every skipped picture is represented, while retaining an empty-reference entry for containers whose reference is unavailable.

Useful? React with 👍 / 👎.

Confirmé (reproduit : un v:group avec rId1 et rId2 ne donnait que rId1). Corrigé dans f0d8fd8 : l'inventaire compte chaque élément qui nomme une image (a:blip, v:imagedata), plus une entrée vide par conteneur extérieur qui n'en nomme aucun (core/document_images.py) ; test test_every_picture_of_a_group_is_inventoried.

Generated by Claude Code

claude · 2026-11-06T14:09:34Z

Review 3 of 3: PR #893 at `f14f905`

Did f14f905 fix the earlier Blocker? Only in part. Review 1 found that the attachments result could grow quadratically with the input. f14f905 removes one cause: each image now carries a section_index instead of a copy of its heading. The other cause is still there, so the Blocker stays open.

Blocker

Nested image elements each copy the same reference, so attachments still grows quadratically (mcp_server/core/docx_parser.py:91-96 and :108-115; mcp_server/core/confluence_parser.py:77-80 and :92-99; sent to the caller at mcp_server/handlers/ingest_document.py:283)

How the copies happen:
- _collect_images walks the whole subtree with elem.iter(). It records every element whose local name is drawing/pict (docx) or image/img (Confluence), including image elements nested inside other image elements.
- _image_reference(node) then returns the first blip/imagedata (docx) or ri:filename/ri:value (Confluence) anywhere under that node.
Example: a chain of N nested <w:pict> (13 bytes per level), or N nested <ac:image> (about 15 bytes per level), wrapping one reference L bytes long. That gives N attachments, each holding the same L-byte string.
Size: with 13N + L ≤ 1 MiB, the worst case is N ≈ 40k and L ≈ 512 KB, about 20 GB of JSON from a 1 MiB input. That is the same size as the Blocker in review 1. A third-party Confluence page is enough to trigger it.
CPU: each _image_reference call walks the shared chain again, so the work is also O(N²).
Before this PR, _count_images returned an integer, so this growth is new in the PR.
How I found it: I traced it by reading the code at f14f905. This job couldn't write or run a script, so I have not reproduced it.
Possible fix: once _collect_images finds an image element, don't descend into it, and have _image_reference ignore image elements nested inside it. Then each reference comes from bytes that belong to one image only, and output size stays linear in the input. If you also cap the reference length, report what was cut rather than cutting silently (CLAUDE.md).

Fix this →

Notes, no action before merge

A caller can't map section_index back to a section. It indexes ParsedDocument.sections, which the result doesn't include, and it doesn't line up with section_memory_ids. For example, in test_ingest_document_attachments.py the "Design" section holds only an image, so index 1 has no matching memory id.
drop_empty_preamble keeps a preamble that holds only an image. This changes nothing in the wiki page or the memories, and the reviewers expect every mutant in the changed functions to be killed by the existing tests.
I didn't check CI on f14f905, and I didn't run lint, pytest or mutmut, because this job wasn't allowed to.

What can merge and what can't

Accepted:
- The section_index change and drop_empty_preamble (mcp_server/core/document_model.py)
- The handler and schema (mcp_server/handlers/ingest_document.py)
- The normalizer
- All test changes
- The hlinkClick fix in f280f3d
Open Blocker: _collect_images and _image_reference in mcp_server/core/docx_parser.py and mcp_server/core/confluence_parser.py. The bug was introduced in eea5b6b and is still there at f14f905. You can fix those two functions here, or send them to a refinement PR.

Verdict: block

chatgpt-codex-connector

🛡️ Codex Security Review · Automatically triggered

Here are some automated security review suggestions for this pull request.

Reviewed commit: f14f905ec0

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

You comment "@codex security review"
A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

chatgpt-codex-connector · 2026-11-06T14:12:32Z

+            out.append(
+                DocumentAttachment(
+                    ordinal=len(out) + 1,
+                    reference=_image_reference(node),


🛡️ Codex Security Review · Automatically triggered

Security: Parse image references in a single pass

When a user ingests an attacker-supplied Confluence export, nested <ac:image> elements make _collect_images call _image_reference() once per ancestor, and each call walks that ancestor's full subtree. This is quadratic: the reproduction parsed 8,000 nodes/168 KB in 2.28 s, while 16,000 nodes/336 KB exceeded 8 s, both below the 1 MiB cap. safe_handler only moves the work to an admitted thread and supplies no deadline, so repeated ingests can consume a core and degrade the local MCP service. Traverse once (the same pattern exists in the DOCX parser), or inspect only allowed direct descendants and cap image count/depth. This is fresh evidence distinct from the prior response-size report: the slowdown occurs before serialization.

SECURITY.md reference: SECURITY.md:L15-L16

Useful? React with 👍 / 👎.

Confirmé sur f14f905, corrigé dans f0d8fd8 : l'inventaire (core/document_images.py) fait un seul parcours itératif en préordre de chaque bloc ; chaque élément est visité une fois et un conteneur imbriqué dans un autre n'est plus relu. Mesuré sur f0d8fd8 (CPython 3.11, parse_confluence_storage) avec N <ac:image> imbriqués autour d'un ri:attachment : 8 000 → 0,019 s, 16 000 → 0,036 s, 60 000 (1,26 Mo) → 0,142 s, 1 entrée à chaque fois. Tests test_nested_confluence_images_around_one_attachment_yield_one_entry et test_nested_containers_around_one_reference_yield_one_entry (docx, 2 000 niveaux).

Generated by Claude Code

…out (#732, slice 1) Review 3 Blocker: the collectors recorded every drawing/pict or image/img in the subtree, nested ones included, and each took the first reference below it, so N nested containers around one L-byte reference gave N copies (quadratic output). Codex: a v:group of pictures gave one entry; section_index could not be resolved by the caller. The inventory now lives in core/document_images.py: one entry per element that names an image (a:blip, v:imagedata, ri:attachment, ri:url, img), plus one per outermost container that names none, read from that element's own attribute in an iterative preorder walk. Output is linear in the input (40,000 nested w:pict around a 500 KB id: 1 entry, 0.06 s). The result carries section_headings once, the table section_index points into. Four equivalent mutants are registered with their reason. Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_018kyZYraH8hqphMRC2MG7Jz

…out (#732, slice 1) unit, run 42233499091: "functions changed, no test file imports mcp_server.core.document_images" (ADR-1122 untested module). The tests reached the module only through the parsers; test_document_images.py now drives collect_images directly and pins a leaf without its attribute (mutants 22/24/25 of _image_references). Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_018kyZYraH8hqphMRC2MG7Jz

cdeust · 2026-11-06T14:49:47Z

unit, run 42233499091 : échec causé par cette PR, corrigé dans a46f55c (fixup! de eea5b6b).

Le gate de mutation (ADR-1122) choisit les tests d'un module par import direct ; test_document_images.py n'atteignait core/document_images.py qu'à travers les parseurs, donc le module est sorti « untested ». Le fichier importe maintenant collect_images/ImageMarkup, teste la numérotation après des entrées existantes et une feuille sans son attribut (mutants 22/24/25 de _image_references, survivants sous cette sélection).

Vérifié localement avec mutmut 3.8.1 sur la sélection exacte de CI (tests_py/core/test_document_images.py) : seuls survivent les quatre équivalents enregistrés dans memory/mutation-equivalents.json. Le script du gate lui-même (uv run) ne tourne pas ici ; pas encore de CI sur a46f55c.

Generated by Claude Code

cdeust · 2026-11-06T15:38:51Z

merge, run 42237375254 : rouge à cause de install (macos-latest) annulé, pas de cette PR. Jobs en échec relancés une fois.

Sur a46f55c, tous les autres checks sont verts (unit, regression, e2e, contract, environment ×3, install Ubuntu/Windows, CodeQL). install (macos-latest) (job 127975518334) a été annulé après 16 min (15:21:46 → 15:37:27 UTC) sans aucune étape enregistrée ni log (l'API renvoie 404 BlobNotFound), et merge échoue sur install: abandoned, needs success. Ce diff ne touche ni .github/, ni scripts/, ni pyproject.toml/uv.lock/ci.json : un job qui meurt avant sa première étape relève du runner, pas du code. J'ai relancé les jobs en échec une fois (rerun-failed-jobs) ; si le job macOS échoue de nouveau, ce sera traité comme un vrai échec.

Generated by Claude Code

…ventory

claude · 2026-11-06T16:22:31Z

Claude finished @cdeust's task in 2m 21s —— View job

Review 4 of 3: PR #893 at `7b47b04`

Is the review 3 Blocker fixed? Yes. The Blocker was that nested image elements each copied the same reference, so the result grew quadratically with the input. f0d8fd8 replaces both _collect_images/_image_reference pairs with one shared function, _image_references, in mcp_server/core/document_images.py:47-73:

It walks the tree once, iteratively, with an explicit stack, so each node is visited once.
Containers nested inside another container are not counted again.
Each entry takes its reference from the attribute of its own leaf element, so no reference is copied.

The result is that output size and CPU time now grow linearly with the input. All five reviewers agreed on this, by reading the code. section_headings (ingest_document.py:249) holds one heading per parsed section. It is taken after drop_empty_preamble, so it lines up with the shifted section_index.

Scope of this review. I reviewed only f0d8fd8, a46f55c and the merge 7b47b04. The merge brings in main and changes none of this PR's lines. I ran five reviews on these commits (code quality, security, performance, test coverage, docs). None found a Blocker:

Rules: layers and # source: comments are fine, and the files stay within the size limits.
Mutation tests: tracing the mutants by hand, the tests kill every mutant of _image_references and collect_images and the section_headings line. The four entries recorded in memory/mutation-equivalents.json are truly equivalent.

Notes, no action before merge

Word saves some pictures twice, as mc:AlternateContent with a w:drawing in mc:Choice and a w:pict in mc:Fallback. Both are counted, giving two entries ([(1,"rId5"),(2,"rId5")]), and a w:pict that holds only a shape adds an empty entry. A later slice could skip mc:Fallback.
Linear growth issues that predate this PR are still open and worth a follow-up: each section memory repeats its full heading path (document_normalizer.py:118-120), Confluence cells re-read nested tables (confluence_parser.py:61-64), and each section body is rebuilt by concatenating every block (docx_parser.py:141, confluence_parser.py:130).
docs/module-inventory.md:192 says "one entry per image-naming element" but leaves out the empty entry for a container with no leaf.

What can merge

The whole PR is accepted. No Blocker is open in any file or commit.

Not done here:

I did not run lint, pytest or mutmut. The mutation check is a hand trace by the reviewers.
I couldn't post this review with gh pr comment: the tool permissions blocked both a temporary file and a multi-line body. The review is here instead.

Verdict: approve

#732, slice 2) (#978) * feat(ingest): each embedded docx image names its part, digest and size (#732, slice 2) The attachment inventory from slice 1 (#893) named a docx image only by its relationship id. The id is now resolved through word/_rels/document.xml.rels (ECMA-376 Part 2 §9.3, Annex A.3) to the package part holding the bytes, and that part is streamed through SHA-256. Linked images, absent parts and Confluence images keep part/digest/size empty; state stays unsupported until a later slice stores the bytes. Malformed relationships fail before any write, like the main part. Refs #732 Co-Authored-By: Claude Opus 5.5 <[email protected]> Claude-Session: https://claude.ai/code/session_018kyZYr4VjkzmxX2THRjfx7 * fixup! feat(ingest): each embedded docx image names its part, digest and size (#732, slice 2) * fixup! feat(ingest): each embedded docx image names its part, digest and size (#732, slice 2) * fixup! feat(ingest): each embedded docx image names its part, digest and size (#732, slice 2) fixup! feat(ingest): each embedded docx image names its part, digest and size (#732, slice 2) unit (run 42750861159): test_every_storage_primitive_is_an_audited_site read archive.open(name) as open-write, the first positional argument of an open on an unresolved receiver being taken as its mode. The member is opened for reading: say so with mode="r". * fixup! feat(ingest): each embedded docx image names its part, digest and size (#732, slice 2) unit (run 42755857850): the mutation gate (ADR-1122) left 12 survivors and found no test importing ingest_document_media. - docx_relationships: the all-'..' guard was redundant (popping word/ already yields ''), so it is now 'if not named'. Id and Target are read without a default and a relationship missing either is skipped; a missing-Target test pins it. - docx_media_reader: the tests assert that the docx path appears in read errors, and that every read asks for at most 64 KiB. mode="r" (the ZipFile.open default) and 'UTF-8' vs 'utf-8' are registered equivalents. - ingest_document_media: the lookup has no default; tests_py/handlers/test_ingest_document_media.py covers it directly. --------- Signed-off-by: Claude <[email protected]> Co-authored-by: Claude <[email protected]>

cdeust mentioned this pull request Nov 5, 2026

feat: preserve document attachments and their provenance through local retrieval #732

Open

5 tasks

fixup! feat(ingest): name each embedded image ingest_document leaves …

cc6f4bf

…out (#732, slice 1)

Merge remote-tracking branch 'origin/main' into fix/732-attachment-in…

0763c5c

…ventory # Conflicts: # mcp_server/core/document_normalizer.py

cdeust marked this pull request as ready for review November 6, 2026 13:50

chatgpt-codex-connector Bot reviewed Nov 6, 2026

View reviewed changes

fixup! feat(ingest): name each embedded image ingest_document leaves …

f280f3d

…out (#732, slice 1)

chatgpt-codex-connector Bot reviewed Nov 6, 2026

View reviewed changes

fixup! feat(ingest): name each embedded image ingest_document leaves …

f14f905

…out (#732, slice 1)

chatgpt-codex-connector Bot reviewed Nov 6, 2026

View reviewed changes

claude added 2 commits November 6, 2026 14:23

cdeust added the claude-rereview ADR-1119: allows one more Claude review (a fourth) on this PR label Nov 6, 2026

Merge remote-tracking branch 'origin/main' into fix/732-attachment-in…

7b47b04

…ventory

github-actions Bot merged commit 6695555 into main Nov 6, 2026
18 checks passed

This was referenced Nov 10, 2026

feat(ingest): each embedded docx image names its part, digest and size (#732, slice 2) #961

Closed

feat(ingest): each embedded docx image names its part, digest and size (#732, slice 2) #978

Merged

Conversation

cdeust commented Nov 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Type of change

Test plan

Audit notes

Breaking changes

Reviewer checklist

Uh oh!

cdeust commented Nov 5, 2026

Uh oh!

chatgpt-codex-connector Bot commented Nov 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Security findings

Advisory findings (2)

Uh oh!

claude Bot commented Nov 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review of PR #893 finished

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Nov 6, 2026

Choose a reason for hiding this comment

Uh oh!

cdeust Nov 6, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot commented Nov 6, 2026

Review 1 of 3: PR #893 at 0763c5c

Blocker

Notes, no action before merge

Uh oh!

claude Bot commented Nov 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR #893 review (1 of 3) done

Uh oh!

claude Bot commented Nov 6, 2026

Review 1 of 3 — PR #893 (head f280f3d)

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

🛡️ Codex Security Review · Automatically triggered

Uh oh!

chatgpt-codex-connector Bot Nov 6, 2026

Choose a reason for hiding this comment

🛡️ Codex Security Review · Automatically triggered

Uh oh!

cdeust Nov 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

claude Bot commented Nov 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review 3 of 3: PR #893 finished

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Nov 6, 2026

Choose a reason for hiding this comment

Uh oh!

cdeust Nov 6, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Nov 6, 2026

Choose a reason for hiding this comment

Uh oh!

cdeust Nov 6, 2026

Choose a reason for hiding this comment

Uh oh!

claude Bot commented Nov 6, 2026

Review 3 of 3: PR #893 at f14f905

Blocker

Notes, no action before merge

cdeust commented Nov 5, 2026 •

edited

Loading

chatgpt-codex-connector Bot commented Nov 6, 2026 •

edited

Loading

claude Bot commented Nov 6, 2026 •

edited

Loading

Review 1 of 3: PR #893 at `0763c5c`

claude Bot commented Nov 6, 2026 •

edited

Loading

Review 1 of 3 — PR #893 (head `f280f3d`)

cdeust Nov 6, 2026 •

edited

Loading

claude Bot commented Nov 6, 2026 •

edited

Loading

Review 3 of 3: PR #893 at `f14f905`

claude Bot commented Nov 6, 2026 •

edited

Loading

Review 4 of 3: PR #893 at `7b47b04`