Repository navigation
fix(spec-sync): pin opus-5, scope V2 wiring to /v2 routes, gate wired paths - #119
Merged
Merged
Conversation
…red paths against the spec Companion to the ade-python fix for landing-ai/ade-python#153 (the AI pass there rewrote the new /v1/classify and /v1/split routes as non-existent /v2/* routes). This repo's runs on the same drift (#117, #118) got it right, but only because the unpinned action happened to default to claude-opus-5[1m]; make that deliberate and add the same guard rails. - Model: pin claude-opus-5[1m] on the V1 wiring, V2 wiring and V2 lint-repair steps (previously unpinned). - Prompt (V2): explicit SCOPE (only /v2/* backs client.v2; /v1/* is the V1 surface, never wired or "translated"; every written path must exist in the snapshot); drop the stale /v1/files -> client.v2.files clause; paging hint for the multi-thousand-line diff; coverage self-check before finishing; dpt-3-fast -> dpt-3-verity wording. - Gate: scripts/spec-sync/check-v2-paths.sh — wired paths ⊆ spec ∪ hidden (build-schema) and spec /v2/* ⊆ wired. Runs in ./scripts/lint (CI lint) and in the spec-sync lint step so the AI repair pass sees it. - Observability: print the agent's narration and tool calls to the step log right after each AI step (jq filter held in the workflow env; log, not artifact, because only logs are secret-masked). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The unresolved critical logging vulnerability and route-gate correctness issues must be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Pins the spec-sync AI model, tightens V2 route scoping, adds route validation, and documents workflow safeguards.
Changes:
- Pins Claude Opus 5 for wiring and repair steps.
- Adds bidirectional V2 route checks to lint and spec-sync.
- Adds AI transcript logging and updates contributor documentation.
Review findings:
- Critical (1 vote) —
.github/workflows/spec-sync.yml:23: Free-form assistant transcript logging can expose transformed, chunked, or encoded secrets. Log only fixed metadata and validated non-sensitive paths. - Moderate (2 votes) —
scripts/spec-sync/check-v2-paths.sh:82: The allowlist accepts snapshot/v1/*routes. Build it from/v2/*plus hidden paths and report out-of-scope paths. - Moderate (1 vote) —
scripts/spec-sync/check-v2-paths.sh:71: Block-comment openers are not excluded, allowing documentation paths to satisfy reverse coverage. - Moderate (1 vote) —
scripts/spec-sync/check-v2-paths.sh:97: Reverse coverage should exclude the hand-maintained/v2/workflow*namespace while retaining forward validation.
File summaries
| File | Description |
|---|---|
scripts/spec-sync/check-v2-paths.sh |
Adds client/spec route consistency checks. |
scripts/lint |
Runs the new route checker. |
CONTRIBUTING.md |
Documents V2 scope and spec-sync safeguards. |
.github/workflows/spec-sync.yml |
Pins models and updates prompts, validation, and transcript logging. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
… stricter path gate - Log only tool name + strictly validated path-like args after each AI step; no assistant text, results, patterns or commands (free-form narration is an exfiltration channel for transformed secrets that log masking cannot catch). Prefix every line so nothing agent-influenced can start a workflow command. - check-v2-paths.sh: forward check accepts only spec /v2/* routes (+ hidden build-schema) and reports a /v1/* route as out of scope; reverse check counts only URL-builder call sites (_v2_url / v2Url), so a doc comment or block-comment mention no longer satisfies coverage; the hand-maintained /v2/workflow* namespace is excluded from the reverse check (its wired paths are still validated forward); `/*` and `/**` comment openers are skipped. File is now identical in both SDK repos. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A critical credential-exposure risk and three moderate correctness and dependency issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 4
- Review effort level: Balanced
…t-aware path gate - Tool-call summary logs only the tool name and argument key names; no argument values at all (a hex/base64-chunked secret in a "path" would pass any character allowlist and defeat log masking). The commit step prints `git diff --cached --stat` from the trusted checkout as the list of what the AI pass changed. - check-v2-paths.sh: the reverse check requires the V2 URL builder to be the first argument of a request method (this._client.get/post<T>(...) / self._get/_post(...)), scanned over comment-stripped, line-joined text so formatter line breaks are handled; an unused v2Url() call no longer counts as "sent". Snapshot `paths` keys are read with jq, python3 or node, whichever is on PATH, so ./scripts/lint needs no new prerequisite. File stays identical in both SDK repos. - Repair prompt repeats the /v2/workflow* hands-off rule so a removed workflow route cannot make the repair pass delete the hand-maintained resource. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Two critical logging vulnerabilities and a moderate route-validation gap remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
.github/workflows/spec-sync.yml:947
- This second
--stathas the same filename exfiltration channel: V2 agents may create arbitrary allowlisted filenames, which this command writes to the log before committing. Log only aggregate counts or omit the stat output so transformed secrets cannot be encoded in a path.
git diff --cached --stat
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Balanced
Comment on lines
+29
to
+30
| | "[tool] " + (.name | tostring | if test("^[A-Za-z]{1,32}$") then . else "<redacted>" end) | ||
| + " " + ((.input // {}) | keys | map(if test("^[A-Za-z_]{1,32}$") then . else "<redacted>" end) | join(",")) |
| run: | | ||
| git add -A | ||
| # Trusted list of what the AI pass changed (the tool-call summary above logs no values). | ||
| git diff --cached --stat |
Comment on lines
+125
to
+126
| grep -vE "$comment_line_re" "$file" | tr '\n' ' ' | grep -oE "$callsite" | grep -oE "$literal" \ | ||
| | sed -E 's/^.//; s/.$//' | normalize || true |
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
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.
Companion to landing-ai/ade-python#155 (root cause of landing-ai/ade-python#153: the AI pass rewrote the new
/v1/classify//v1/splitroutes as non-existent/v2/*routes and skippedpasswordon/v2/parse). This repo's runs on the same drift (#117, #118) did the right thing — but only because the unpinned action happened to default toclaude-opus-5[1m]. This makes that deliberate and identical across both SDKs, and adds the same guard rails.--model "claude-opus-5[1m]"on the V1 wiring, V2 wiring and V2 lint-repair steps (previously unpinned → floats with the Claude Code release)./v2/*backsclient.v2; the AIDE spec's/v1/*routes are the V1 surface, never wired or "translated"; every written path must exist in the snapshot. Drops the stale/v1/files→client.v2.filesclause (gone since spec-sync: track V2 spec drift #93), adds a paging hint for the multi-thousand-line diff and a coverage self-check before finishing;dpt-3-fast→dpt-3-veritywording (matches ade-python).scripts/spec-sync/check-v2-paths.sh(identical file in both repos). Forward: every/vN/literal a V2 resource uses must be a/v2/*spec route or the hidden build-schema surface — a/v1/*route is reported as out of scope, a missing one as not in the spec. Reverse: every/v2/*spec route outside the hand-maintained/v2/workflow*namespace must be sent —this._client.post(this.v2Url(...))/.get(...), matched across line breaks — so a doc-comment mention or an unusedv2Url()call does not count; the wired workflow paths are still validated forward. Reads the snapshot with jq, python3 or node, whichever is on PATH (no new prerequisite). Runs in./scripts/lint(CIlint) and inside the spec-sync lint step so the AI repair pass sees it. Passesmainand spec-sync(v2): parse: add password support for encrypted pdfs #118.git diff --cached --statfrom the trusted checkout. No agent-controlled value is logged: a prompt-injected agent could encode a credential into narration or a file path and carry it past secret masking. Thejqfilter lives in the workflow's top-levelenvand runs before any AI-authored code can execute.