Inject package.json dependency allowlist into generation context - #157
Merged
Conversation
…neration context The generation prompt's HARD GUARDRAIL against introducing new external npm packages was abstract with nothing concrete to check against, and the local coding model still imported a nonexistent package during benchmarking. Concrete data beats an abstract rule for a small local model's instruction following. buildFileContentsBlock() now reads the target repo's package.json (when present) and prepends a formatted "Allowed npm dependencies" list — merging dependencies and devDependencies — ahead of the existing file-context block. Absent or malformed package.json resolves to null (not an error), since this pipeline is also used against non-Node.js repos. Companion to #155, which updates generation-user.md/generation-system.md to reference this block. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
3 tasks
![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: 2bf4958a84
ℹ️ 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".
- Include peerDependencies and optionalDependencies in the allowlist, not just dependencies/devDependencies — both are valid package.json declaration fields; omitting them could make the generation guardrail reject or rewrite a valid import. - Add a targeted test for the unexpected non-ENOENT read-error branch (package.json as a directory, throwing EISDIR) to confirm it's rethrown rather than swallowed, per this repo's per-branch test coverage policy. - Document the dependency-allowlist injection behavior in docs/code-generation.md (new "Dependency Allowlist Injection" section). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e97468d37a
ℹ️ 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".
- Soften the allowlist wording: it reflects the repository ROOT manifest only and does not override the pre-existing, separate allowance for a package already imported elsewhere in the target file (a workspace/ monorepo package could legitimately import something declared in its own nested package.json, or a file could already import something not in the root manifest) — the previous "exhaustive"/"do not import outside this list" framing contradicted that existing rule. - Cap the injected list at 200 entries (MAX_DEPENDENCIES), truncated alphabetically with a note for larger manifests, mirroring the existing MAX_FILES/MAX_FILE_SIZE bounding pattern in this same module — an unbounded dependency table could otherwise blow the generation prompt's context budget and abort generation entirely. - 3 new tests cover the truncation boundary and the softened wording. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
koydas
added a commit
that referenced
this pull request
Jul 27, 2026
docs/code-generation.md described the dependency-allowlist injection as active pipeline behavior and linked to docs/adr/0019-static-verification-backstop.md, but neither the injection code (scripts/lib/file_injector.mjs) nor the ADR file exist in this branch — they're delivered by companion PRs #157 and #158, which are separate, not-yet-merged branches. Reworded to describe what THIS prompt change expects/consumes, attribute the actual implementation to PR #157 by reference, and point the ADR mention at PR #158's URL instead of a dead relative path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1de654f4fd
ℹ️ 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".
koydas
added a commit
that referenced
this pull request
Jul 27, 2026
…rty crashes (#155) * feat(prompts): sharpen generation guardrails against native-API imports and read-only property crashes Benchmarking the local coding model against these exact prompts surfaced two concrete failure modes: it imported a package (`abort-controller`) for functionality (`AbortController`) that's a native global, and it assigned to `AbortController.signal`, a getter-only property, causing a guaranteed runtime TypeError in strict-mode ES modules. Neither was caught by the existing "never introduce a new external package" guardrail because it had no concrete allowlist to check against, and there was no explicit self-verification step for read-only property assignment. Also generalizes the test-writing requirement (previously scoped only to scripts/, prompts/, .github/workflows/) to apply whenever the issue requests tests or new non-trivial logic is introduced, regardless of path — the narrower scoping let a requested test file for a new hook get silently dropped. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(prompts): address review feedback on generation guardrails - Correct an accuracy bug in the guardrail wording itself: Node built-in modules (fs, path, node:*) still require an explicit import/require statement — they were incorrectly grouped with true no-import globals (AbortController, fetch). Distinguish the two categories explicitly. - Add 7 targeted prompt-regression tests covering the new dependency allowlist reference, the no-import-vs-importable-built-in distinction, the path-independent test-writing requirement, and both self-check clauses (read-only property, cross-render persistence) — previously only covered by generic non-empty/schema-key assertions that couldn't catch removal of these specific guarantees. - Document the expanded generation guardrails in docs/code-generation.md (new "Generation Guardrails" section), since this changes the operator- visible contract of what the pipeline generates. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(docs): stop asserting cross-PR behavior not present in this tree docs/code-generation.md described the dependency-allowlist injection as active pipeline behavior and linked to docs/adr/0019-static-verification-backstop.md, but neither the injection code (scripts/lib/file_injector.mjs) nor the ADR file exist in this branch — they're delivered by companion PRs #157 and #158, which are separate, not-yet-merged branches. Reworded to describe what THIS prompt change expects/consumes, attribute the actual implementation to PR #157 by reference, and point the ADR mention at PR #158's URL instead of a dead relative path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…ncy-allowlist # Conflicts: # docs/code-generation.md
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c41d58516
ℹ️ 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".
koydas
added a commit
that referenced
this pull request
Jul 27, 2026
…lformed-manifest bug - has_test_file_changes was computed from extractChangedFiles, which also includes files that were only DELETED (their path appears via the --- a/... line). A PR that deletes a test file alongside an untested behavioral change would incorrectly set this to true. Added extractAddedOrModifiedFiles (only +++ b/... lines, excluding /dev/null deletions) and use it for this computation instead. - TEST_PATTERNS required a leading slash before test(s)/__tests__/, so root-level paths like test/foo.js or tests/foo.js (no leading slash) were never recognized as test files. Anchored the patterns to also match at the start of the path. - has_test_file_changes: true was being treated as conclusive proof of coverage, but it only means "some test file was touched somewhere" — an unrelated or stub test edit would still satisfy it. Restored reviewer judgment: when the test file's content is visible in the diff, the reviewer must still assess relevance; the boolean is only trusted outright when diff_truncated prevents that judgment. - scripts/lib/dependency_manifest.mjs had the exact same malformed-package.json bug already fixed in file_injector.mjs (PR #157) — a consequence of intentionally duplicating this logic to keep the two PRs independent. Applied the identical isPlainObject/asDependencyMap guard here. - 11 new unit tests + 2 prompt-regression tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The static prompt text (generation-system.md/generation-user.md) tells the model to treat the injected "Allowed npm dependencies" block as exhaustive. When the manifest exceeds MAX_DEPENDENCIES (200) and the list is truncated alphabetically, a genuinely-declared dependency sorting after entry 200 would be missing from what the model sees, causing it to wrongly reject or rewrite a valid import. The truncation note now explicitly overrides the static "exhaustive" framing for that specific request, instructing the model not to reject an import solely for absence from a truncated list. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a90d4b7c5
ℹ️ 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".
koydas
added a commit
that referenced
this pull request
Jul 27, 2026
…w/auto-fix prompts (#156) * feat(prompts): generalize test-coverage gate and add named defect checklist to review/auto-fix Benchmarking the local coding model as both coder and reviewer on the same production prompts found the reviewer approved a diff containing a guaranteed-crash bug (assignment to a read-only property), an explicitly banned external dependency, and zero test coverage for an issue that explicitly requested tests — verdict: APPROVED, Issues Found: None. Two root causes addressed here: 1. The existing gates (a)/(b)/(c) for test coverage only apply to automation paths (.github/workflows/, scripts/, prompts/, docs/code-generation.md). A feature PR for arbitrary application code has no equivalent rule, even when the reviewer's own classification header says "Tests expected: yes" — generalized the test-coverage check to trigger off tests_expected directly, regardless of path. 2. The review rubric was abstract ("report real bugs, broken logic"), which this model did not reliably apply to two specific, previously-observed failure modes. Added a named checklist for read-only property assignment, unauthorized dependency imports, and non-persistent ref/counter patterns — mirroring the auto-fix-system.md guardrails so the reviewer checks for the same defects the generator is asked to avoid. Also hardens auto-fix-system.md with the same two guardrails (native-API awareness, read-only property self-check) plus path-independent test inclusion, so a fix pass doesn't reintroduce what it's meant to repair. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(prompts): address review feedback on review/auto-fix guardrails - Fix an accuracy bug in auto-fix-system.md: Node built-in modules (fs, path, node:*) still require an explicit import/require statement — they were incorrectly grouped with true no-import globals (AbortController, fetch). Distinguish the two categories, same fix applied to generation-system.md in the companion PR. - Add 6 targeted prompt-regression tests covering the generalized test-coverage gate, the named defect checklist in pr-review-system.md, and the three hardened guardrails in auto-fix-system.md (native-API distinction, path-independent test inclusion, read-only/persistence self-check) — previously only covered by a generic non-empty assertion. - Document the new review/auto-fix guardrails in docs/code-generation.md (new "Review and Auto-Fix Guardrails" section), since this changes the operator-visible review contract. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(pr-review): supply real package.json context for the dependency check The "Unauthorized dependency" check added to pr-review-system.md instructed the reviewer to verify imports against package.json, but pr_review.mjs only ever supplied the diff plus issue metadata — never unchanged file content. A dependency already declared in an untouched package.json was therefore unverifiable, and the reviewer had no way to actually enforce this rule. Adds scripts/lib/dependency_manifest.mjs (readPackageJsonDependencies, formatDependencyManifestContext, buildDependencyManifestContext) and wires it into pr_review.mjs's prompt construction, reading the PR branch's local package.json (already checked out by pr-review.yml) and appending a "Declared npm dependencies" block the reviewer can check imports against. 11 new unit tests cover the missing/malformed/empty/populated manifest cases, formatting, and the unexpected-read-error rethrow branch. Also fixes a broken relative link to docs/adr/0019-static-verification-backstop.md (that file lives on a separate not-yet-merged PR's branch) — points to the source PR instead until it merges. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(pr-review): preserve test-file evidence and named-check coverage past the diff truncation cutoff pr_review.mjs truncates the diff shown to the model to 12,000 chars (filterDiff), but the new test-coverage gate and named defect checklist implicitly assumed the model could see the whole diff. Two concrete gaps: 1. tests_expected is computed from the full untruncated rawDiff, but the gate then asked the model to "verify the diff actually adds or updates a test file" by looking at the (possibly truncated) diff text — a real test-file change past the cutoff would be invisible, producing a false "missing tests" finding. 2. The named defect checks (read-only property, unauthorized import, non-persistent ref) can only inspect what's shown; a defect in a later hunk was silently invisible, and the review gave an unqualified APPROVED implying full coverage. Fixes: - change_classifier.mjs: exports isTestFile; buildChangeClassificationContext now also computes has_test_file_changes from the full changedFiles list (already derived from the untruncated rawDiff) and accepts a diffTruncated flag, both surfaced in the classification context block. - pr_review.mjs: computes diffTruncated by comparing the untruncated filtered-diff length against the 12,000-char truncated version, passes it into buildChangeClassificationContext. - pr-review-system.md: the test-coverage gate now trusts has_test_file_changes over what's visible in the diff text; when diff_truncated is true, the reviewer must disclose that only a partial diff was inspected rather than imply full coverage in its verdict. - 8 new unit tests (change_classifier) + 2 new prompt-regression tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(pr-review): tighten has_test_file_changes and fix a duplicated malformed-manifest bug - has_test_file_changes was computed from extractChangedFiles, which also includes files that were only DELETED (their path appears via the --- a/... line). A PR that deletes a test file alongside an untested behavioral change would incorrectly set this to true. Added extractAddedOrModifiedFiles (only +++ b/... lines, excluding /dev/null deletions) and use it for this computation instead. - TEST_PATTERNS required a leading slash before test(s)/__tests__/, so root-level paths like test/foo.js or tests/foo.js (no leading slash) were never recognized as test files. Anchored the patterns to also match at the start of the path. - has_test_file_changes: true was being treated as conclusive proof of coverage, but it only means "some test file was touched somewhere" — an unrelated or stub test edit would still satisfy it. Restored reviewer judgment: when the test file's content is visible in the diff, the reviewer must still assess relevance; the boolean is only trusted outright when diff_truncated prevents that judgment. - scripts/lib/dependency_manifest.mjs had the exact same malformed-package.json bug already fixed in file_injector.mjs (PR #157) — a consequence of intentionally duplicating this logic to keep the two PRs independent. Applied the identical isPlainObject/asDependencyMap guard here. - 11 new unit tests + 2 prompt-regression tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…d of staying silent
formatDependencyAllowlist treated a real package.json declaring zero
dependencies identically to "no manifest at all" (both returned ''). But
these are different: an empty {} is a meaningful, exhaustive answer ("no
npm packages are allowed"), while null means the manifest couldn't be read
at all. Staying silent for the empty case meant the model was never told
zero imports are permitted, defeating the entire guardrail for exactly the
repos with no runtime dependencies — including this one, per AGENTS.md's
"no external runtime dependencies" convention. Now emits an explicit
"(none — package.json declares zero dependencies)" block with a hard
"do not introduce ANY new external npm package" instruction; only a
genuinely unreadable/missing manifest returns ''.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b560aace7
ℹ️ 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".
…ust entry count MAX_DEPENDENCIES=200 alone doesn't bound the block's actual size: 200 long scoped package names (@org/long-package-name style) can still add tens of KB on top of the existing file-context budget (MAX_FILES * MAX_FILE_SIZE = 80,000 chars). The generation stage has no token-budget truncation the way auto-fix does (autofix_max_input_tokens) — it only logs an estimate — so this block needs to bound itself. Added MAX_ALLOWLIST_CHARS (4000), truncating on whichever limit (count or characters) is reached first. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
generation-system.md/generation-user.mdto reference an "Allowed npm dependencies" context block; this PR actually populates it.buildFileContentsBlock()(used bygenerate_issue_change.mjs) now reads the target repo'spackage.jsonwhen present and prepends a formatted allowlist (mergeddependencies+devDependencies) ahead of the existing file-context block. This gives the model concrete data to check imports against instead of only an abstract "don't add new packages" rule — during benchmarking, the abstract rule alone did not stop the model from importing a nonexistentabort-controllerpackage for functionality (AbortController) that's a native global.package.jsonresolves tonull, not an error/throw — this pipeline also runs against non-Node.js repos, so a missing manifest is an expected case, not a failure.file_injector.mjsonly (used by the generation stage).auto_fix_pr.mjsbuilds its file-context block with separate inline logic from a changed-files list rather than issue-text extraction — wiring the same allowlist there would touch an entrypoint script's more sensitive flow; flagging as a possible follow-up rather than bundling it into this PR.Test plan
node --test scripts/tests/file_injector.test.mjspasses (37/37, 11 new tests covering: missing/malformed/empty/populated package.json, allowlist formatting, and integration withbuildFileContentsBlock).node --test scripts/tests/*.test.mjs— full suite passes (545/545).Companion to #155 and #156.
🤖 Generated with Claude Code