Repository navigation
fix(spec-sync): scope V2 wiring to /v2 routes, gate wired paths, pin opus-5 - #155
Conversation
…st the spec, pin opus-5 Root cause of #153: the AI pass rewrote the AIDE spec's new /v1/classify and /v1/split routes as client.v2.classify/split hitting non-existent /v2/* paths (staging 404, contract gate red) and skipped the in-scope `password` field on /v2/parse. Nothing said what to do with non-/v2 routes in this spec, and nothing checked that a wired path exists. - Prompt: 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. - Gate: scripts/spec-sync/check-v2-paths.sh — wired paths ⊆ spec ∪ hidden (build-schema) and spec /v2/* ⊆ wired ∪ deferred (workflow). Runs in ./scripts/lint (CI lint) and in the spec-sync lint step so the AI repair pass sees it. Rejects #153's tree; passes main. - Model: pin every AI step to claude-opus-5[1m] (was claude-opus-4-8[1m]), the same pin ade-typescript now carries. - 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>
There was a problem hiding this comment.
🟡 Changes recommended
Critical transcript-logging risks and substantive V2 path-gate gaps remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Hardens V2 spec-sync routing, validation, model selection, and observability.
Changes:
- Adds V2 route validation to lint and spec-sync.
- Tightens prompts, pins Opus 5, and logs transcript summaries.
- Documents V2-only routing scope.
File summaries
| File | Review |
|---|---|
scripts/spec-sync/check-v2-paths.sh |
Gate incorrectly permits /v1/*, can mistake docstrings for wired routes, and overstates successful coverage. |
scripts/lint |
Runs the new route gate; no additional issues found. |
docs/design/v2-spec-sync-extension.md |
Documents the revised routing policy; no issues found. |
CONTRIBUTING.md |
Documents spec-sync safeguards; no issues found. |
.github/workflows/spec-sync.yml |
Critical: raw, attacker-influenced transcripts can inject workflow commands and expose transformed credentials in logs. Sanitize output and avoid publishing free-form transcripts from credentialed runs. |
Review details
Suppressed comments (1)
scripts/spec-sync/check-v2-paths.sh:112
- This success message says every
/v2route is wired, but the check deliberately considersDEFERREDroutes covered as well; with the current workflow entries it overstates the result on every successful run. Report them as “covered (wired or deferred)” instead.
echo "check-v2-paths: OK — $(wc -l < "$tmp/wired" | tr -d ' ') wired path(s) all present in $spec; all $(wc -l < "$tmp/spec_v2" | tr -d ' ') /v2 spec route(s) wired."
- Files reviewed: 5/5 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 docstring or block-comment mention no longer satisfies coverage; the /v2/workflow* namespace is excluded from the reverse check (not auto-wired by design); `/*` and `/**` comment openers are skipped. File is now identical in both SDK repos. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The critical secret-exposure risk and moderate lint and route-scanning issues must be resolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 3
- 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 (self._get/_post(...) / this._client.get/post(...)), scanned over comment-stripped, line-joined text so formatter line breaks are handled; an unused builder 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 resource. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The workflow has a critical logging exposure, and both logging allowlists and route detection require correction.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
.github/workflows/spec-sync.yml:30
- Argument names are still model-controlled JSON keys, not fixed metadata. A prompt-injected agent can encode a credential into alphabetic key names (for example, base-26 chunks under 32 characters); the regex accepts them and publishes them from the transcript even if the resulting tool call is invalid. Filter keys through an explicit allowlist of the known schemas for Edit/Write/Read/Glob/Grep/Bash, and likewise map tool names to a fixed allowlist, rather than validating arbitrary model output by character class.
TOOL_CALL_SUMMARY_JQ: >-
(if type == "array" then .[] else . end)
| select(.type == "assistant") | .message.content[]?
| select(.type == "tool_use")
| "[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(","))
.github/workflows/spec-sync.yml:899
- This also logs agent-controlled filenames from the V2 pass. The allowlist restricts directories but still lets the agent create arbitrary names under
src/,tests/, ordocs/, so a transformed credential can be encoded into a permitted filename and emitted bygit diff --cached --stat. Keep these paths out of the workflow log.
# Trusted list of what the AI passes changed (the tool-call summaries above log no values).
git diff --cached --stat
scripts/spec-sync/check-v2-paths.sh:126
- The reverse check still accepts a docstring that contains a request-shaped example. After lines are joined, text such as
"""Example: self._post(self._v2_url("/v2/foo"))"""matches this pipeline and makes/v2/fooappear wired even though no request exists. I verified that this fixture exits 0 against a spec containing only/v2/foo. Strip strings/comments structurally (while retaining URL literals that are actual call arguments), or parse Python/TypeScript call expressions, before populatingsent.
grep -vE "$comment_line_re" "$file" | tr '\n' ' ' | grep -oE "$callsite" | grep -oE "$literal" \
| sed -E 's/^.//; s/.$//' | normalize || true
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
| # Trusted list of what the AI pass changed (the tool-call summary above logs no values). | ||
| git diff --cached --stat |
Root cause of #153: the AI commit wired
client.v2.classify/client.v2.splitagainst/v2/classifyand/v2/split, which the spec does not have (staging 404 → contract tests red), and skipped the one in-scope change, the newpasswordfield on/v2/parse. The drift only added/v1/*routes (/v1/classify*,/v1/split,/v1/ade/*); the prompt never said what to do with non-/v2routes in the AIDE spec, and nothing checked that a wired path exists./v2/*backsclient.v2;/v1/*routes in this spec 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 #129), adds a paging hint for the multi-thousand-line diff and a coverage self-check before finishing.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 not-auto-wired/v2/workflow*namespace must be sent —self._post(self._v2_url(...))/self._get(...), matched across line breaks — so a docstring mention or an unused builder call does not count. Reads the snapshot with jq, python3 or node, whichever is on PATH (no new prerequisite). Runs in./scripts/lint(CIlint, a required check) and inside the spec-sync lint step so the AI repair pass sees it. Rejects spec-sync(v2): track spec drift #153's tree with exactly the four invented paths; passesmain.claude-opus-5[1m](wasclaude-opus-4-8[1m]). ade-typescript pins the same in fix(spec-sync): pin opus-5, scope V2 wiring to /v2 routes, gate wired paths ade-typescript#119 — it was unpinned, ran opus-5 by default, and wired this very drift correctly twice.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.Follow-up once merged: close #153 so the next hourly run re-wires the drift under the new prompt and gate.