Repository navigation
feat(codemode): versioned libraries and std v1 pure helpers - #812
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 17 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: f149bc5a65
ℹ️ 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".
| } | ||
| if (typeof target === "function") { | ||
| const bound = (input) => target(mergeInput(defaults, input)); | ||
| for (const key of Object.keys(target)) bound[key] = bind(target[key], defaults); |
There was a problem hiding this comment.
name or length
When a tool namespace contains a child named name or length, std.bind throws while constructing the wrapper: every non-root ToolReference reports typeof === "function", but the new arrow function already has read-only name and length properties. Consequently, otherwise valid tools such as tools.api.name cannot be used through a bound namespace, and binding fails before any tool executes. Define child properties explicitly with descriptors rather than assigning over the wrapper's read-only function metadata.
Useful? React with 👍 / 👎.
| const freezeLibraryValue = (value: Value): Value => { | ||
| if (!(value instanceof Obj)) return value | ||
| value.extensible = false | ||
| if (value instanceof Bytes && value.bytes.length > 0) { | ||
| throw new TypeError("Cannot freeze array buffer views with elements.") |
There was a problem hiding this comment.
packages/codemode/AGENTS.md explicitly requires: “Ask the object instead of adding instanceof ladders.” This new helper instead dispatches on Obj, Bytes, and Arr, duplicating the integrity-level implementation in src/stdlib/object.ts. That creates a second object-kind checklist that must stay synchronized whenever freezing behavior changes. Route library freezing through shared object-owned integrity behavior rather than introducing another subclass-dispatch ladder.
AGENTS.md reference: packages/codemode/AGENTS.md:L5-L5
Useful? React with 👍 / 👎.
| if (!libraryNamePattern.test(library.name)) | ||
| throw new TypeError(`Library name "${library.name}" must be a valid JavaScript identifier.`) |
There was a problem hiding this comment.
When a configured library name is a reserved word such as class or return, this character-only check accepts it even though program code cannot reference it as an identifier. The accepted name this is worse: it is absent from globalNames, so defineGlobal overwrites the interpreter's existing this binding, changing top-level this from undefined to the library export. Validate names as JavaScript binding identifiers, including reserved-word rejection, so these configurations fail at make as the API promises.
Useful? React with 👍 / 👎.
|
The following comment was made by an LLM, it may be inaccurate: |
- Reject reserved words (incl. this) as library names so a library cannot overwrite the top-level this binding or install an unreferenceable global. - std.bind: fall back to the namespace-object form when a child named name/length cannot be assigned over a function wrapper read-only metadata, so such namespaces bind instead of throwing. - Route library freezing through the shared SetIntegrityLevel implementation in stdlib/object (freezeValue) instead of a duplicate subclass ladder in execute.ts. - Tests: reserved-name rejection, top-level this stays undefined, binding namespace children named name and length.
|
Status by queue worker (lane A), task 2982: read the sealed unit logs from run 40807380452 and addressed the Codex review on PR #812; new head Unit failures on the previous head were both outside codemode (PR touches only codemode + core codemode files):
All three Codex findings were reproduced in the interpreter first, then fixed:
Local verification on the new head: packages/codemode Resume: when run 40815525561 finishes, if unit failures are only these same unrelated flakes, merge PR #812 per the standing directive, confirm #802 closed, complete task 2982 — that also unblocks parked task 2964 (#803 phase 2). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93cec21bf1
ℹ️ 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".
| const out = {}; | ||
| for (const key of Object.keys(target)) out[key] = bind(target[key], defaults); | ||
| return out; |
There was a problem hiding this comment.
When a tool is also a namespace with a child named name or length, this fallback preserves the children but discards the parent tool's callability. toolTrie in src/tool-runtime.ts supports nodes containing both a tool and children, so a configuration such as { api: echo, "api.name": echo } is valid; however, const bound = std.bind(tools.api, defaults); await bound(input) now fails because bound is a plain object. The new collision fallback fixes the previously reported child-access failure but introduces this separate failure for callable parent nodes. Keep the callable wrapper and define colliding child properties with descriptors instead.
Useful? React with 👍 / 👎.
| function iterateeFn(iteratee, label) { | ||
| if (typeof iteratee === "function") return iteratee; | ||
| if (typeof iteratee === "string") { | ||
| return (item) => (item !== null && typeof item === "object" ? item[iteratee] : undefined); |
There was a problem hiding this comment.
For arrays of strings, the property-name shorthand silently uses undefined for every key because this guard excludes primitives, even though the interpreter supports string properties such as length and indexed characters. Consequently, std.uniqBy(["a", "bb", "cc"], "length") returns only ["a"], whereas the equivalent callback (s) => s.length returns ["a", "bb"]; std.keyBy(["a", "bb"], "length") similarly collapses both entries into the "undefined" key. The advertised property-name iteratee should perform the property lookup for non-nullish primitives too, rather than silently discarding their distinct keys.
Useful? React with 👍 / 👎.
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
|
Lane A resumed task 2982, verified this PR, and merged it:
agent-note:v1 agent=repo-queue-worker-shift-opencode-a task=2982 issue=802 pr=812 stage=completed-merged |
Issue for this PR
Closes #802
Type of change
What does this PR do?
Implements #802 (phase 1 of design #800): CodeMode gains a versioned program-source
librariesoption. Each library is parsed and evaluated separately so program diagnostics keep their own line/column, then bound to one frozen global. Ships std v1.0.0 pure helpers (bind, chunk, uniqBy, keyBy, pick, omit, version) evaluated inside the interpreter;std.bindwraps tool callables/namespaces and records as the original tool. Core wiring passes the std library and the instructions render the std@1.0.0 listing; interpreter-support.md gains the library checklist entries. Phase 2 async helpers (#803) are explicitly out of scope.Follow-up commit addressing automated review findings, each reproduced in the interpreter before fixing: reserved words (including
this) are rejected as library names — a library namedthispreviously overwrote the top-levelthisbinding;std.bindnow binds namespaces whose children are namedname/lengthinstead of throwing on a function wrapper's read-only metadata; library freezing routes through the shared SetIntegrityLevel implementation in stdlib/object instead of a duplicate subclass ladder.How did you verify your code works?
bun testin packages/codemode: 1409 pass, 0 fail (incl. new libraries tests for the review fixes).bun testin packages/core (codemode catalog/instructions/tool-execute): 29 pass, 0 fail.bun run checkfrom repo root: 36/36 tasks pass.Screenshots / recordings
Not a UI change.
Checklist
Need help on this PR? Tag
@codesmith-botwith what you need.