Repository navigation
fix: address the retroactive review findings on the v1.6.1 port - #291
Conversation
The Vue extractor mints its script-block functions itself and still called generate_node_id directly, so two same-name function declarations on one line shared an id and the store kept one row. Upstream reaches the allocator by delegating Vue scripts to its tree-sitter extractor. Route the script functions through NodeIdAllocator, with the block-relative line and UTF-16 column the delegated extractor would use; a non-colliding id is unchanged.
Since exported stores own their inline actions, the walker skipped the
rest of the initializer, so `create(persist(...), { storage:
createJSONStorage(...) })` recorded no call to `create`, `persist` or
`createJSONStorage`, which the port recorded before. Upstream skips the
initializer the same way. Walk the initializer attributed to the store
and skip only the extracted action values, so no action's calls are
counted twice, and walk the factory closure as the store's body instead
of binding a curried `create<S>()(...)` factory as a function named
after the store.
Vue same-line ids and store initializer calls change what extraction emits, so an index built at version 17 is reported outdated and re-extracted.
A gap marker names the indexed symbols it hides only from the budget the file has spare; a name that costs more leaves a bare `... (gap) ...`. The note closing the response still said the gap markers named what was elided, on both tiers. Report a withheld name from the render, and say the markers name only what room allowed when one was withheld. The epilogue reserve holds the longer of the two wordings.
…ponent
A default import resolved to the module's first exported component
before the binding its `export default NAME` statement names. The React
resolver mints component nodes inside ordinary JS/TS modules, so a
module exporting `function Screen() { return <div /> }` beside `const
Api = { upload }; export default Api` bound `import Api` to `Screen`:
`Api.upload()` became a call to `Screen` and lost its `upload` edge.
Upstream orders the lookup the same way. Try the stated binding first,
then a component (a Svelte or Vue file has no such statement and is its
own default export), then the first exported function or class.
Seven merged port PRs were reviewed by kirocodex after the fact. The entry lists each verdict, the four defects #291 fixes (three of them KEEP-RUST divergences), and the findings kept at upstream behavior with upstream's reasons.
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #291 +/- ##
==========================================
+ Coverage 95.29% 95.31% +0.01%
==========================================
Files 161 161
Lines 99084 99130 +46
==========================================
+ Hits 94422 94485 +63
+ Misses 4662 4645 -17
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
The previous commit switched the trim note to a hedged wording only when an inner gap marker withheld a name. The tail marker after a dropped or windowed cluster names nothing either, and a dropped file appears only in the pointer list, so the unhedged wording was still false on most trimmed responses. Use the hedged wording on both tiers always, and drop the withheld-name plumbing that chose between the two. Cover the tail marker on both tiers.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60d441e772
ℹ️ 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".
| self.file_path, | ||
| NodeKind::Function, | ||
| &name, | ||
| start_pos.row as u32 + 1, |
There was a problem hiding this comment.
For a valid SFC containing both <script> and <script setup>, functions with the same name at the same block-relative row and column receive the same ID even though their actual source lines differ; the allocator treats the second declaration as a revisit, and the store upsert drops one node. Pass the line including block.line_offset, matching the Node.start_line, so IDs follow the required file-relative formula.
AGENTS.md reference: AGENTS.md:L46-L52
Useful? React with 👍 / 👎.
| let previous = self.store_walk.replace(StoreWalk { | ||
| factory: nearest_function_ancestor(actions).map(|factory| factory.id()), | ||
| actions: actions_ids, | ||
| }); |
There was a problem hiding this comment.
When the returned action object contains a function under an unsupported key such as [dynamic]: () => wrongCall() or a numeric key, extract_js_object_literal_members does not mint a node or include its syntax ID in actions_ids. The subsequent initializer walk therefore descends into that closure and attributes wrongCall to the store, even though it runs only when the action is invoked; track and skip all callable member values, not only successfully extracted actions.
Useful? React with 👍 / 👎.
| let (mut file_section, names_withheld) = | ||
| join_parts_with_named_gaps(file_path, &parts, &file_index_nodes, spare); |
There was a problem hiding this comment.
When a file is clipped because a single cluster is windowed or another cluster is omitted, join_parts_with_named_gaps can return names_withheld = false because it only examines gaps between rendered parts, after which the renderer appends a separate bare ... (gap) .... The epilogue then uses the full wording claiming elided symbols are named inside gap markers even though this trailing marker names nothing; include this clipped marker in the flag or name its omitted symbols, and cover this branch as part of the MCP text contract.
AGENTS.md reference: AGENTS.md:L62-L64
Useful? React with 👍 / 👎.
The trim note now hedges on both tiers always, rather than only when an inner gap withheld a name.
Rust 1.96's clippy, which CI pins, rejects !opt.is_some_and(p); write it as opt.is_none_or(!p).
1 participant
Summary
The owner asked for a retroactive kirocodex review of the large v1.6.1 port PRs that merged before the review rule existed: #271, #275, #277, #278, #279, #281 and #282. Each was reviewed by the goal gate against its own parent. #271, #275 and #279 passed. #277, #278, #281 and #282 failed with ten findings in total.
Each finding was checked against current
main(c00aa2b) and upstreamv1.6.1(f4ddf50). This PR fixes the four that are real defects in the port and bumps the extraction version. The other six are left as they are, with reasons below.Fixed
generate_node_iddirectly. Two same-name declarations on one line shared an id, so the store kept one row. Upstream reaches its allocator by delegating Vue scripts to the tree-sitter extractor. The fix routes them throughNodeIdAllocatorwith the block-relative line and UTF-16 column that the delegated extractor would use. Non-colliding ids are unchanged.create(persist(..., { storage: createJSONStorage(...) }))then recorded no call tocreate,persistorcreateJSONStorage. The port recorded those calls before fix(resolve): v1.6.1 receiver evidence, store and object-literal bindings, and extraction recall #277; upstream skips them too.create<S>()(...)factory therefore no longer becomes a function named after the store.export default NAMEstatement names. The React resolver mints component nodes inside ordinary.js/.tsmodules, soimport Api from './screen'bound toScreenin a module that exportsfunction Screen() { return <div /> }besideconst Api = { upload }; export default Api.Api.upload()then became a call toScreen, and theuploadedge was lost. Upstream orders the lookup the same way. The stated binding now comes first, then a component (a Svelte or Vue file has no such statement and is its own default export), then the first exported function or class.Declined, with reasons
.getState()chains bind for a store built by any factory, not only ZustandmatchStoreAccessorChainchecks provenance only for selectors. The member must sit in that holder's own object literal, and the call site must call.getState()on that holder. A provenance gate would also dropzustand/vanilla, wrapped (createSelectors(create(...))) and locally re-exported factories.#ifconditions are unknowndefined(), or a bare name. An unknown condition keeps the call, which was the behavior before #2069.#ifndef X/ empty#define Xis read as a guardguardsItself. A default value (#define X 0) stays unknown.initializer_listconstructor declines the overload setVerification
red-vue-identity.logandred-vue-store.log: the store kept 1 row where there should be 2;red-store-initializer.log;red-unnamed-gap-note.log: both tiers;red-default-binding.log:consumecalledsrc/screen.js::Screen.r2-red-trailing-gap.log: the tail marker case on both tiers, against round 1's code.mutation-factory-guard.logandmutation-action-skip.logshow that each guard in the store walk is load-bearing.60d441ewith a release build; every corpus isidentical(golden-drift.log).make pre-ciat60d441e, clean tree: 3969 tests passed, 0 failed, and all checks passed (pre-ci.log). That target is what the pre-push hook runs. It was rerun on the final head (pre-ci-r2.log).60366e4fixes;!opt.is_some_and(p)(nonminimal_bool),59a2520rewrote it asopt.is_none_or(!p). A fresh review of the cumulative diff at59a2520passed with no blocking items.make pre-cion Rust 1.96.0 at59a2520, clean tree: 3971 passed, 0 failed (pre-ci-r3.log). The earlier runs used 1.98, because this shell exportsRUSTUP_TOOLCHAIN.All logs are under
/tmp/evidence-retro/.BEGIN_COMMIT_OVERRIDE
fix(extract): give same-line Vue script functions distinct ids (#1349)
fix(extract): keep a store initializer's own calls on the store
fix(resolve): prefer an explicit default binding over an exported component
fix(mcp): never claim that gap markers name every elided symbol
END_COMMIT_OVERRIDE
🤖 Generated with Claude Code